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

Respect Context Deadline #185

Merged
merged 3 commits into from
Feb 19, 2017
Merged

Conversation

RaniSputnik
Copy link
Contributor

@RaniSputnik RaniSputnik commented Feb 8, 2017

Have added context timeout to graphql.Execute, this seemed the most appropriate place to put it. A couple of concerns I have;

  • Nested resolve functions could still be started after a context has already been cancelled
  • Not sure of the performance implications of these changes - doesn't look too bad to me but I hasn't been benchmarked
  • The go func inside of Execute's pretty messy now, the same select is repeated three times
  • The tests run slower after this change as they need to wait for the context timeout. I've kept this to 100ms for now, so not a massive deal - but it might be better to kick off this test in parallel.

Feedback very much appreciated!

Closes #184

@coveralls
Copy link

coveralls commented Feb 8, 2017

Coverage Status

Coverage increased (+0.05%) to 83.532% when pulling 3a996ee on RaniSputnik:context-deadline into 22050ee on graphql-go:master.

@coveralls
Copy link

coveralls commented Feb 8, 2017

Coverage Status

Coverage increased (+0.04%) to 83.529% when pulling e95c164 on RaniSputnik:context-deadline into 22050ee on graphql-go:master.

@chris-ramon
Copy link
Member

chris-ramon commented Feb 19, 2017

Have added context timeout to graphql.Execute, this seemed the most appropriate place to put it. A couple of concerns I have;

Nested resolve functions could still be started after a context has already been cancelled
Not sure of the performance implications of these changes - doesn't look too bad to me but I hasn't been benchmarked
The go func inside of Execute's pretty messy now, the same select is repeated three times
The tests run slower after this change as they need to wait for the context timeout. I've kept this to 100ms for now, so not a massive deal - but it might be better to kick off this test in parallel.
Feedback very much appreciated!

Thanks a lot @RaniSputnik!

Nested resolve functions could still be started after a context has already been cancelled

I think it's fine, if graphql results contain errors, clients will either retry or something else.

Not sure of the performance implications of these changes - doesn't look too bad to me but I hasn't been benchmarked

I don't think there are perf. implications, if so we will handle it on new PR's

The go func inside of Execute's pretty messy now, the same select is repeated three times

Could not agree more, looking forward to code clean-up inside the Execute function.

The tests run slower after this change as they need to wait for the context timeout. I've kept this to 100ms for now, so not a massive deal - but it might be better to kick off this test in parallel.

IMHO 100ms is fine for now.

LGTM 👍 🚢

@chris-ramon chris-ramon merged commit a77f106 into graphql-go:master Feb 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Context Should Respect Deadline
3 participants