Skip to content

Conversation

@ahmadizm
Copy link

The different code path that existed prior to this change would omit the calls to onFieldValuesInfo when an UnresolvedTypeException occurred, and that causes a thread leak similar to an older issue for unhandled exceptions #2068 - that is, once a type cannot be resolved, the deeper batches from other (successful) branches are never dispatched.

Since we are just adding an error in the cases of UnresolvedTypeException and try to set the value to null, this only needed a small refactoring to make it similar to when the value is actually null.

The different code path that existed prior to this change would omit the calls to `onFieldValuesInfo` when an `UnresolvedTypeException` occurred, and that causes a thread leak similar to an older issue for unhandled exceptions graphql-java#2068 - that is, once a type cannot be resolved, the deeper batches from other (successful) branches are never dispatched.

Since we are just adding an error in the cases of `UnresolvedTypeException` and try to set the value to null, this only needed a small refactoring to make it similar to when the value is actually null.
@bbakerman bbakerman added this to the 20.1 milestone Mar 18, 2023
@bbakerman bbakerman self-requested a review March 18, 2023 10:27
Copy link
Member

@bbakerman bbakerman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

private FieldValueInfo getFieldValueInfoForNull(ExecutionContext executionContext, ExecutionStrategyParameters parameters) {
CompletableFuture<ExecutionResult> fieldValue = completeValueForNull(executionContext, parameters);
return FieldValueInfo.newFieldValueInfo(NULL).fieldValue(fieldValue).build();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a look at this in the complete code. I can see that the value as not being completed into a CF and hence this would intere with the completion of data loaders and so on.

@bbakerman bbakerman added this pull request to the merge queue Mar 18, 2023
@bbakerman bbakerman merged commit 7f0e4de into graphql-java:master Mar 18, 2023
@ahmadizm ahmadizm deleted the typeErrorHandling branch March 21, 2023 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants