Fixed reflect.Value.Type on zero Value panic when subscription resolver itself panics
#417
Conversation
This file contains 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
…lver itself panicks The internal exec Subscribe method had code to deal with subscription resolver panicking when being called. But when such handling happen, the error is attached to the request object and it never checked later on. This leads to some zero checks to fail when we try to extract the type from the resolver's channel since this variable was never set. Doing this creates a second panic which is not handled and make the application die. To fix the issue, we now check if there is errors on the request object before continuing with the rest of the check, if there is errors, it's because a panic occurs and we return the response right away.
maoueh
commented
Nov 11, 2020
| @@ -37,7 +37,7 @@ type extensionser interface { | |||
| } | |||
|
|
|||
| func makePanicError(value interface{}) *errors.QueryError { | |||
| return errors.Errorf("graphql: panic occurred: %v", value) | |||
There was a problem hiding this comment.
This was causing panic errors to print like graphql: graphql: ... so I removed it. Not other tests failed, so I assume it was never checked.
Can revert is asked for it no problem.
|
@maoueh there seems to be a conflict in this PR. Can you, please, fix it. |
# Conflicts: # subscription_test.go
|
Conflicts fixed |
|
Thank you |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The internal exec Subscribe method had code to deal with subscription resolver panicking when being called. But when such handling happen, the error is attached to the request object and it never checked later on.
This leads to some zero checks to fail when we try to extract the type from the resolver's channel since this variable was never set. Doing this creates a second panic which is not handled and make the application die.
To fix the issue, we now check if there is errors on the request object before continuing with the rest of the check, if there is errors, it's because a panic occurs and we return the response right away.