Fixes #2068 - Silent thread leak on exception in completeValueForList#2359
Merged
bbakerman merged 2 commits intographql-java:masterfrom Aug 3, 2021
Merged
Fixes #2068 - Silent thread leak on exception in completeValueForList#2359bbakerman merged 2 commits intographql-java:masterfrom
bbakerman merged 2 commits intographql-java:masterfrom
Conversation
dfa1
reviewed
May 24, 2021
| .type(newTypeWiring("Query") | ||
| .dataFetcher("pets", new StaticDataFetcher(['cats': cats, 'dogs': dogs]))) | ||
| .type(newTypeWiring("Cat") | ||
| .dataFetcher("toys", new StaticDataFetcher(new List<Object>() { |
Contributor
There was a problem hiding this comment.
maybe a Collections.singletonList is better here? size returns 1, but then toArrray, listIterator returns both null
Contributor
There was a problem hiding this comment.
my bad, just spotted the "iterator" that throws exception!
Contributor
There was a problem hiding this comment.
@KammererTob what about:
new java.util.AbstractList<>() {
@Override
public int size() {
return 1;
}
@Override
public Object get(int index) {
throw new RuntimeException("simulated failure");
}
};
by default, the Iterator will call get(), throwing exception, like in your code.
Contributor
Author
There was a problem hiding this comment.
Thanks for the feedback. I've adjusted the code as by your comment and i also renamed the whole test block to better reflect what it is testing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This an attempt to fix #2068 . It might be a bit too naive, but i wanted to get another set of eyes on this issue.
Here my reasoning for this fix:
As explained in the issue, the problem lies in how the dataloader determines when to dispatch different levels. When one of the
resolveFieldWithInfofutures completes exceptionally, theAsync.eachFuture also completes exceptionally and the dataloader instrumentation is not made aware of this. SinceexpectedStrategyCallsPerLevelis increased by calls tohandleOnFieldValuesInfoby previous levels andallOnFieldCallsHappenedis checked for the previous level when dispatching, it means that all deeper levels will never be dispatched (expectedStrategyCallsPerLevelis bigger thanhappenedOnFieldValueCallsPerLevel) and we leak a thread. The changes made make sure that the instrumentation is made aware of this exceptional completion and thus that deeper levels can still be dispatched.If you have better ideas to fix this i am open for your suggestions! Thanks!