Make network timeouts configurable

[sidonath]

Timeouts to external services should be a concern of a client, as it could affect its reliability and performance.

Unfortunately, in the current architecture, it's not possible to cleanly change timeouts from (very generous, if I may say) defaults.

This PR attempts to make network timeout settings configurable, relying on the existing plumbing for configuring the client.

I couldn't think of a clean way to write a spec that asserts timeouts actually get applied to Net::HTTP, but it's rather easy to see in console:

c = Intercom::Client.new(token: token)
opts = Intercom::Client.set_timeouts(open_timeout: 0.1, read_timeout: 0.1)
c.options(opts)
c.admins.all.to_a
# Intercom::ServiceConnectionError: Failed to connect to service [connection attempt timed out]

[theandrewykim]

Hey @sidonath - thanks for your contribution! To be totally honest, I'm not quite sure what problem you're handling with the code in this PR. From what I understand our API doesn't allow any configurable timeouts, so this isn't something that can be changed through this client either.

Your specs don't seem to handle any change in behavior - just that the settings you've adjusted have been properly set as well. I'm going to close this PR for now, but in the chance that I'm wrong(entirely possible) - please feel to re-open.

[sidonath]

Hi @theandrewykim 👋

To be totally honest, I'm not quite sure what problem you're handling with the code in this PR.

The problem is that intercom-ruby is setting network timeouts to a very generous value. We wanted to lower the timeouts within our application to accommodate our use-case and make sure that the requests to Intercom don't take more time than we're ready to give it (let's say ~10s). We're fine with the requests failing due to that timeout.

However, because of the way timeouts for Net::HTTP are set, we're not able to change them from our application:

https://github.com/sidonath/intercom-ruby/blob/f3448ffe01e934e771ed7af818034330e837e48b/lib/intercom/request.rb#L49-L59

This PR simply attempts to provide the public API to configure the timeouts.

From what I understand our API doesn't allow any configurable timeouts, so this isn't something that can be changed through this client either.

Right, I never thought the timeout can be set through the Intercom API, it's a network-related technicality 😊

Your specs don't seem to handle any change in behavior - just that the settings you've adjusted have been properly set as well.

That's all the PR attempts to do, yes 😄

I'd really appreciate if you'd reconsider merging the PR, because otherwise we'd be forced to use the fork and keep it up to date. If you see any downsides of making the network timeouts configurable in this way, please let me know and maybe we can come up with a different solution!

As I'm not a contributor, I'm afraid I can't reopen this PR 😞

[eugeneius]

Hi @sidonath, thanks for explaining why you need this functionality. I agree that the network timeouts should be configurable, so I'll reopen this pull request for you.

On the implementation - what do you think of passing the timeouts to request.execute here:

intercom-ruby/lib/intercom/client.rb

Line 113 in f3448ff

request.execute(@base_url, username: @username_part, secret: @password_part)

That way they don't have to be threaded through get/post/put/delete in both Client and Request.

[sidonath]

Thank you @eugeneius!

That's a great idea and makes the change much smaller. I guess I missed that execution path 😅 I've updated the PR with the new approach.

[jonnyom]

Hey @sidonath thanks for updating this. I'm happy to approve this for you now 👍