Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[Bug]: Ignoring an element from a tuple yielded from an iterator results in memory leaks #25926

Open
jabraham17 opened this issue Sep 10, 2024 · 0 comments

Comments

@jabraham17
Copy link
Member

The following code does not correctly call the destructors for the objects allocated by the iterators and may result in leaked memory

class C {
  var id: int;
  proc deinit() {
    writeln("deinit C id=", id);
  }
}

record R {
  var id: int;
  proc deinit() {
    writeln("deinit R id=", id);
  }
}

iter foo() {
  yield (new C(1), new C(2));
  yield (new C(3), new C(4));
}

iter bar() {
  yield (new R(1), new R(2));
  yield (new R(3), new R(4));
}


for (_, x) in foo() {
  writeln(x);
}
for (_, x) in bar() {
  writeln(x);
}

The correct output is as follows

{id = 2}
deinit C id=2
deinit C id=1
{id = 4}
deinit C id=4
deinit C id=3
(id = 2)
deinit R id=2
deinit R id=1
deinit R id=2
deinit R id=1
(id = 4)
deinit R id=4
deinit R id=3
deinit R id=4
deinit R id=3

However, the deinit for the first tuple component does not get invoked. This is caused by the ignored tuple component, _. In the case of heap memory (i.e. classes or strings), then this causes memory to be leaked

Changing the bottom two loops to use a dummy variable results in correct execution:

for (dummy, x) in foo() {
  writeln(x);
}
for (dummy, x) in bar() {
  writeln(x);
}

This is related to #25893, where ignoring a yielded variable results in deinit not being called

jabraham17 added a commit that referenced this issue Sep 24, 2024
…nd LockFreeQueue (#25928)

Resolves all of the memory leaks that are exposed by our current tests
in the ConcurrentMap, EpochManager, LockFreeStack, and LockFreeQueue
modules

While working on this, I also found
#25926 and this PR adds a
future for that.

Testing
- [x] full paratest with/without comm
- [x] full paratest with -memleaks
- [x] full paratest with comm and -memleaks

[Reviewed by @stonea]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant