Add Timeout Logic #36
Conversation
| module Graph | ||
| class << self | ||
| def query(query_string, variables: {}, context: {}, timeout: nil) | ||
| Timeout.timeout(timeout) do |
There was a problem hiding this comment.
😯 http://www.mikeperham.com/2015/05/08/timeout-rubys-most-dangerous-api/
Does that apply here? (That's the reason I didn't use the built-in API for graphql-ruby's timeout middleware)
There was a problem hiding this comment.
Funny I had the same discussion with @cjoudrey yesterday.
Dylan showed me this https://github.com/ruby/ruby/blob/ruby_2_3/lib/net/http.rb#L878
There was a problem hiding this comment.
Also it seems like for web requests in the worse case unicorn will kill the process. I guess I probably wouldn't use it in library code though 🤔
There was a problem hiding this comment.
What's the takeway from that? Maybe I'm missing it...
That code is opening a connection, right? So it times out, but you don't have a dangling connection or any in-progress operations.
But during the GraphQL query, there might some in-progress network operations when you abort (as described in the blog post) but then the connection gets reused by the next HTTP request, and 😿 !
(I've never seen this bug in the wild, but it was the impression I got from that blog post.)
@cjoudrey