Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Memory error with Future if result is never retrieved #15117

Closed
mppf opened this issue Mar 6, 2020 · 0 comments · Fixed by #15118
Closed

Memory error with Future if result is never retrieved #15117

mppf opened this issue Mar 6, 2020 · 0 comments · Fixed by #15118

Comments

@mppf
Copy link
Member

mppf commented Mar 6, 2020 •
edited
Loading

The following program shows memory errors with valgrind:

use Futures;
use Time;

config const doGet = false;

proc test() {
  var F = async(lambda(x: int) { sleep(1); return 42 + x; }, 23);
  if doGet then
    F.get();
}

proc main() {
  test();
}

These errors go away if the --doGet=true argument is passed.

Why is this happening? The async call creates a future and returns it. The future destructor doesn't wait for the task and cleans up the memory. Then, the task finishes and tries to store the result in the future's state.

Associated Future Test:

test/library/packages/Futures/bug-15117.chpl (PR #15118)
@mppf mppf mentioned this issue Mar 6, 2020
Merged
2 tasks
mppf added a commit that referenced this issue Mar 6, 2020
@mppf
Merge pull request #15118 from mppf/follow-15041-futures
20e1d58 
Fix two problems in the Futures package module

This PR fixes two memory errors in the Futures package module.

Resolves #15117.

The first is described in issue #15117. This PR fixes that case by
waiting for the future to complete if it was not completed when the
future is trying to delete the class managing it. A reasonable
alternative would be to increment the reference count to that class in
the task running the future.

The second issue is a problem that surfaces due to PR #15041 that
appeared as a test failure for futures-doc-validity.chpl. The problem is
with constructions like this:

``` chapel
// from assign-then-get.chpl
var G: Future(int);
assert(G.isValid() == false);
G = async(lambda(x: int) { return 10 + x; }, 1);  // HERE
const g = G.get();
```

The problem is that the first-class-function is deinited at the end of
the statement in which it was declared, at the `HERE` comment above. To
fix that, made the futures functions accept task function arguments with
the `in` intent and pass them to the begin also with `in` intent.

Reviewed by @vasslitvinov - thanks!

- [x] test/library/packages/Futures passes with valgrind
- [x] full local testing
# for free to join this conversation on GitHub. Already have an account? # to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix two problems in the Futures package module mppf/chapel
1 participant
@mppf