Add timeout option

[neroleung]

I wanted to set a longer timeout for background jobs but a shorter timeout for synchronize requests. This allows users to specify a timeout if they so desire.

[dblock]

The current mechanism to do this would be

Graphlient::Client.new('http://test-graphql.biz/graphql') do |client|
      client.http do |h|
        h.connection do |c|
          c.timeout = 20
        end
      end
    end

I can see how specifying the timeout in the client constructor is easier. But I worry that we're forcing all adapters into supporting timeout, so the next person will want read_timeout vs. write_timeout and then we open a can of worms for all the options.

What do you think about generalizing access to adapter options instead?

client = Graphlient::Client.new('https://test-graphql.biz/graphql',
  headers: {
    'Authorization' => 'Bearer 123'
  },
  http_options: {
    timeout: 20
  }
)

client = Graphlient::Client.new('https://test-graphql.biz/graphql',
  headers: {
    'Authorization' => 'Bearer 123'
  },
  http_options: {
    read_timeout: 20,
    write_timeout: 30
  }
)

[yuki24]

I'm in favor of @dblock's second suggestion.

[neroleung]

Thanks for the suggestions. I've change the code to do http_options - read_timeout and write_timeout.

[dblock]

I believe this doesn't work when timeouts are set to 0 or nil, which are both valid values for options. Check everywhere for key presence in the options.

Now, this is better from the interface POV but we can be future-proof and client-agnostic with a bit of meta programming.

Instead of

          Net::HTTP.new(uri.host, uri.port).tap do |client|
            client.use_ssl = uri.scheme == 'https'
            client.read_timeout = http_options[:read_timeout] if http_options[:read_timeout]

            if client.respond_to?(:write_timeout=)
              client.write_timeout = http_options[:write_timeout] if http_options[:write_timeout]
            end

Do

          Net::HTTP.new(uri.host, uri.port).tap do |client|
            client.use_ssl = uri.scheme == 'https'
            http_options.each_pair do |k, v|
               client.send("#{k}=", v)
            end

Check in tests that we get a proper/useful error when trying to assign write_timeout to a client that doesn't support it, too. Might need some better rescue in the actual implementation.

[neroleung]

@dblock, for the meta-programming part, did you mean to do it in just http_adapter.rb, or both http_adapter.rb and faraday_adapter.rb?

[dblock]

@dblock, for the meta-programming part, did you mean to do it in just http_adapter.rb, or both http_adapter.rb and faraday_adapter.rb?

Both. I think Graphlient should serve as a passthrough to whatever options users specify, and not try to be smart about timeout options.

[neroleung]

Updated the code with a shared private method #configure_http_options. Please let me know what you think. Thx~

[dblock]

This needs to be failing loudly, we want callers to know that they are trying to set an option that is not supported. Specialize an exception, rescue NameError here and raise that exception. Make sure to add a test.

[neroleung]

Rescuing NoMethodError and re-raise a new Graphlient::Errors::HttpOptionsError now. Thx for all the feedback!

[dblock]

Updated the code with a shared private method #configure_http_options. Please let me know what you think. Thx~

Almost there. Let's fail loudly on invalid options and we're good to go.

[dblock]

👍