Skip to content
This repository was archived by the owner on Sep 24, 2019. It is now read-only.

Add Timeout Logic #36

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions app/controllers/graphql_controller.rb
Original file line number Diff line number Diff line change
@@ -1,14 +1,20 @@
class GraphqlController < ApplicationController
GRAPHQL_TIMEOUT = 10

before_action :authenticate

def execute
query_string = params[:query].to_s
variables = ensure_hash(params[:variables])
context = {
user: @user
}
context = { user: @user }

result = Graph.query(
query_string,
variables: variables,
context: context,
timeout: GRAPHQL_TIMEOUT
)

result = Graph::Schema.execute(query_string, variables: variables, context: context)
render json: result
end

Expand Down
8 changes: 8 additions & 0 deletions app/models/graph.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
require 'timeout'

module Graph
class << self
def query(query_string, variables: {}, context: {}, timeout: nil)
Timeout.timeout(timeout) do

Choose a reason for hiding this comment

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

😯 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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 🤔

Choose a reason for hiding this comment

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

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.)

Graph::Schema.execute(query_string, variables: variables, context: context)
end
end

def find_by_id_field(type, model)
GraphQL::Field.define do
type type
Expand Down