You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by aaronjwood <gi...@git.apache.org> on 2016/08/13 07:54:50 UTC
[GitHub] mesos pull request #157: Prevent memory leaks
GitHub user aaronjwood opened a pull request:
https://github.com/apache/mesos/pull/157
Prevent memory leaks
This should prevent any of the promises that are created in the various `ZookeeperProcess` class methods from leaking memory.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aaronjwood/mesos master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/mesos/pull/157.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #157
----
commit 5fed9cfee4d655c1fb052d8498c4df868046dc7d
Author: Aaron Wood <aa...@verizon.com>
Date: 2016-08-13T06:31:55Z
Added info to contributors.
commit 5d2bea834a94f8c8719648793cb3cb34070f66fa
Author: Aaron Wood <aa...@verizon.com>
Date: 2016-08-13T07:32:00Z
Prevent memory leak by keeping the promise local.
commit d96b748805c878ec80ed9b55a1412730974a91b1
Author: Aaron Wood <aa...@verizon.com>
Date: 2016-08-13T07:37:01Z
Prevent memory leak by keeping the promise local.
commit 12ff3675acf7387d8c1c6d04e2db38265c25334d
Author: Aaron Wood <aa...@verizon.com>
Date: 2016-08-13T07:39:40Z
Prevent memory leak by keeping the promise local.
commit c5f6d0426bab1ec1afb3f66851d21bb014c83d81
Author: Aaron Wood <aa...@verizon.com>
Date: 2016-08-13T07:42:15Z
Prevent memory leak by keeping the promise local.
commit ca39579c17476b7132149da2fb800e109332d4cc
Author: Aaron Wood <aa...@verizon.com>
Date: 2016-08-13T07:42:51Z
Prevent memory leak by keeping the promise local.
commit 4aba5ab5911973fc939f92ce13d38d43be9258c9
Author: Aaron Wood <aa...@verizon.com>
Date: 2016-08-13T07:43:28Z
Prevent memory leak by keeping the promise local.
commit 526c9c477829a4683db78938043ce39f1f044fed
Author: Aaron Wood <aa...@verizon.com>
Date: 2016-08-13T07:44:04Z
Prevent memory leak by keeping the promise local.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] mesos issue #157: Prevent memory leaks
Posted by haosdent <gi...@git.apache.org>.
Github user haosdent commented on the issue:
https://github.com/apache/mesos/pull/157
In addition, I think it would be more helpful to describe more context about the memory leaks here to help others understand this patch.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] mesos issue #157: Prevent memory leaks
Posted by xiaods <gi...@git.apache.org>.
Github user xiaods commented on the issue:
https://github.com/apache/mesos/pull/157
could you please describe the context about memory leaks and give some testing to confirm the root cause.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] mesos issue #157: [MESOS-6038] Prevent memory leaks
Posted by aaronjwood <gi...@git.apache.org>.
Github user aaronjwood commented on the issue:
https://github.com/apache/mesos/pull/157
No no, that's okay. I appreciate everyone helping me validate these changes. I'm trying to be very careful with this :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] mesos issue #157: [MESOS-6038] Prevent memory leaks
Posted by haosdent <gi...@git.apache.org>.
Github user haosdent commented on the issue:
https://github.com/apache/mesos/pull/157
>Even though the Promise is local to the stack, when we return Future shouldn't it be a copy of what's needed underneath?
Sorry, I misunderstanding something before. Future store its data in `std::shared_ptr<Data> data` and your function return a copy instead of const reference, so it should work.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] mesos issue #157: [MESOS-6038] Prevent memory leaks
Posted by Vish0007 <gi...@git.apache.org>.
Github user Vish0007 commented on the issue:
https://github.com/apache/mesos/pull/157
@aaronjwood the "promise" object you have has local scope (and lifetime) and its not valid outside the method; I am sure valgrind wold report this as error;
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] mesos issue #157: [MESOS-6038] Prevent memory leaks
Posted by aaronjwood <gi...@git.apache.org>.
Github user aaronjwood commented on the issue:
https://github.com/apache/mesos/pull/157
It looks like I was wrong about this, I had thought the copy constructor of tuple was getting called when it wasn't the copy constructor at all. I did some more testing with this and found that the original version behaves properly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] mesos issue #157: Prevent memory leaks
Posted by aaronjwood <gi...@git.apache.org>.
Github user aaronjwood commented on the issue:
https://github.com/apache/mesos/pull/157
For the context: I had found a few places where these `Promise` objects were leaking. Once the `Future` was returned there was no way to free the memory for the `Promise` object that had been allocated thus causing the leak. I don't have any direct issues with running Mesos that stemmed from this, it's just something that I thought could be fixed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] mesos issue #157: Prevent memory leaks
Posted by haosdent <gi...@git.apache.org>.
Github user haosdent commented on the issue:
https://github.com/apache/mesos/pull/157
Hi, @aaronjwood Because we use [reviewboard](https://reviews.apache.org/groups/mesos/) to review code changes. Could you follow https://github.com/apache/mesos/blob/master/docs/submitting-a-patch.md to submit your patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] mesos issue #157: [MESOS-6038] Prevent memory leaks
Posted by aaronjwood <gi...@git.apache.org>.
Github user aaronjwood commented on the issue:
https://github.com/apache/mesos/pull/157
I might be misunderstanding things but why can we not do this? I see it being done here https://github.com/apache/mesos/blob/master/3rdparty/libprocess/src/http.cpp#L1001
If we make the `Promise` local like it is in this patch, pass its address into the tuple via the copy constructor (that should be the copy constructor getting called, right?) so that it takes a copy and doesn't rely on an actual address from the stack, and return the future in the same way as the example above, would that not work? Even though the `Promise` is local to the stack, when we return `Future` shouldn't it be a copy of what's needed underneath?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] mesos issue #157: [MESOS-6038] Prevent memory leaks
Posted by aaronjwood <gi...@git.apache.org>.
Github user aaronjwood commented on the issue:
https://github.com/apache/mesos/pull/157
I don't have access to their Slack but I agree.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] mesos issue #157: Prevent memory leaks
Posted by aaronjwood <gi...@git.apache.org>.
Github user aaronjwood commented on the issue:
https://github.com/apache/mesos/pull/157
Hi @haosdent. Sure, I'm sorry. I had created a JIRA and reviewboard account and went to open this PR but should have only included the change to the contributors file.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] mesos issue #157: [MESOS-6038] Prevent memory leaks
Posted by haosdent <gi...@git.apache.org>.
Github user haosdent commented on the issue:
https://github.com/apache/mesos/pull/157
+1 For what @Vish0007 said. If promise became a locale scope object here, return its future seems incorrect. I search `return \S+\.future\(\);` in all exists code, all `Promise` are member objects or heap objects.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] mesos issue #157: [MESOS-6038] Prevent memory leaks
Posted by haosdent <gi...@git.apache.org>.
Github user haosdent commented on the issue:
https://github.com/apache/mesos/pull/157
You could join via https://mesos-slackin.herokuapp.com/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] mesos issue #157: [MESOS-6038] Prevent memory leaks
Posted by haosdent <gi...@git.apache.org>.
Github user haosdent commented on the issue:
https://github.com/apache/mesos/pull/157
By the way, copy a comment from @jpeach in https://mesos.slack.com/messages/cxx/
```
Promise<int> promise = Promise<int>();
why not just
```Promise<int> promise;```
``
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] mesos pull request #157: [MESOS-6038] Prevent memory leaks
Posted by aaronjwood <gi...@git.apache.org>.
Github user aaronjwood closed the pull request at:
https://github.com/apache/mesos/pull/157
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---