GH-5159 close child of GroupIterator #5160
Conversation
| try { | ||
| cf.close(); | ||
| } finally { | ||
| argumentsIter.close(); |
There was a problem hiding this comment.
Looks like you need to add:
var argumentsIter = this.argumentsIter;
if(argumentsIter != null) argumentsIter.close() There was a problem hiding this comment.
Obviously! Thanks Håvard.
Should we check if the current iterator is closed within the loop? We have the isClosed() method we can call. For performance reasons the backing variables isn't volatile, but it should still get updated eventually. The variable is set before the handleClose() method is called, so it would be a nice safety net in case the argumentsIter doesn't close correctly for some reason. |
|
Thanks for this fix. Very good catch and a nice and clean solution! |
Yes, that's good to add. |
frensjan
force-pushed
the
GH-5159-close-group-iterator-arg
branch
from
9f6e7e9
to
6d97c2a
Compare
|
LGTM! thank you @frensjan |
| cf.close(); | ||
| } finally { | ||
| if (argumentsIter != null) { | ||
| argumentsIter.close(); |
There was a problem hiding this comment.
Cant argumentsIter still be null? It could be set to null between this line and the null check above.
There was a problem hiding this comment.
Yes, indeed. Missed that in your initial comment.
| iteratorThread.start(); | ||
| assertThat(iterating.await(5, TimeUnit.SECONDS)).isTrue(); | ||
| groupIterator.close(); | ||
| assertThat(closed.await(5, TimeUnit.SECONDS)).isTrue(); |
There was a problem hiding this comment.
Can we also assert that the iteratorThread is no longer alive? So we are sure that we're not leaking a thread.
There was a problem hiding this comment.
The test was in fact leaking a Thread. Just fixed it.
frensjan
force-pushed
the
GH-5159-close-group-iterator-arg
branch
from
6d97c2a
to
c58c287
Compare
frensjan
force-pushed
the
GH-5159-close-group-iterator-arg
branch
from
c58c287
to
384d1b5
Compare
|
Thanks @frensjan ! I've merged the PR and it'll ve part of the next bug fix release which is due any day now. |
GitHub issue resolved: #5159
Briefly describe the changes proposed in this PR:
I've added the child argument's iterator as (volatile) field in the GroupIterator so that it can be closed while building entires.
PR Author Checklist (see the contributor guidelines for more details):
mvn process-resourcesto format from the command line)