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

Docs missing for setting recommended timeouts #493

Open
bf4 opened this issue Feb 6, 2020 · 7 comments
Open

Docs missing for setting recommended timeouts #493

bf4 opened this issue Feb 6, 2020 · 7 comments
Labels
difficulty: easy fix is easy in difficulty status: help wanted requesting help from the community type: community enhancement feature request not on Twilio's roadmap type: docs update documentation change not affecting the code

Comments

@bf4
Copy link

bf4 commented Feb 6, 2020

Issue Summary

We just got an 'execution expired' net open timeout from twilio (Twilio::REST::TwilioError) , which is fine, it happens, but because we're on Heroku, we need to limit requests to less than 30 seconds else they close it. I saw an error in the log and the customer said the request took a very long time (we validate phone numbers on some object creation using the lookup api).

Which got me to thinking:

  1. twilio-ruby should recommend shorter than default timeouts
  2. twilio-ruby should document how to set timeouts both globally and more specifically
    related to Add a global configuration for the HTTP client #467 Unhandled Faraday::Connection Failed #455

Steps to Reproduce

run an app for while and once in a blue moon get a server time out.

Code Snippet

globally maybe do

Faraday.options.timeout = 20       # open/read timeout in seconds
Faraday.options.open_timeout = 1.5 # connection open timeout in seconds

twilio specific would have something to do with the http client

Exception/Log

see related issues

Twilio::REST::TwilioError: execution expired (Most recent call first)
Faraday::ConnectionFailed: execution expired (Most recent call first)
Net::OpenTimeout: execution expired (Most recent call first)

Technical details:

  • twilio-ruby version: 5.31.1
  • ruby version: 2.6.2
  • environment: Heroku
@childish-sambino childish-sambino added difficulty: easy fix is easy in difficulty status: help wanted requesting help from the community type: community enhancement feature request not on Twilio's roadmap type: docs update documentation change not affecting the code labels Feb 7, 2020
@childish-sambino
Copy link
Contributor

Makes sense to me. Pull requests and +1s on the issue summary will help it move up the backlog.

@bf4
Copy link
Author

bf4 commented Feb 10, 2020

@childish-sambino thanks. I I couldn't figure out from a quick code read and poking at it how to configure the twilio http client globally

@mlapping
Copy link

data point for whoever does this: https://github.com/ankane/the-ultimate-guide-to-ruby-timeouts/blob/master/test/twilio_test.rb

@bf4
Copy link
Author

bf4 commented Jun 11, 2020

seems related to https://github.com/twilio/twilio-ruby/pull/514/files #513 where the answer can be something like

client = Twilio::REST::Client.new(ENV["TWILIO_ACCOUNT_SID"], ENV["TWILIO_AUTH_TOKEN"]).tap do |c|
  if c.http_client.timeout.nil?
    c.http_client.instance_variable_set(:@timeout, 20)
    raise ArgumentError, "Expected to set timeout to 20" if c.http_client.timeout != 20
  end
end

bearing in mind that twilio doesn't distinguish the two timeout types and doesn't provide a writer for either

          f.options.open_timeout = request.timeout || @timeout
          f.options.timeout = request.timeout || @timeout

@bf4
Copy link
Author

bf4 commented Jun 16, 2020

Hijacking my own issue since related

So, I shouldn't assume I want to retry the request, no?

How long long to wait before I could reliably check the messages API for the absence of the sent text to mean it wasn't sent. Query by message id immediately after creating a message is basically immediately available, but query by number and date-sent are more likely to be 'eventually consistent', per CAP theorem

I have an open support ticket about this ( 4562575 ) but it's going back and forth so I thought I'd share the situation here in case it helps anyone (including myself)

@bf4
Copy link
Author

bf4 commented Jul 5, 2020

What I ended up writing in my support ticket before closing it

  1. Twilio usually responds within a second
  2. We need requests to be quick because e.g. for phone number lookup we have to complete the request within 30 seconds due to heroku router constraints
  3. We've set the http client time out to 10 seconds which seems reasonable
  4. When we get an execution expired due to http client timeout on message send, it's possible that twilio eventually processed the request.
  5. We don't want to send the message twice.
  6. So, the issue I created is to try to handle this situation.. asked in terms of how long to wait before checking in the API if twilio subsequently sent the text nonetheless.

It seems to me right now that as long as we make transactional requests in a background worker, we want to let twilio timeout the connection and hence set the http client timeout to 30 seconds.

For regular API requests, like phone number validation, 10 seconds is fine. There's a huge unknown period of time between 10 and 30 (15 and 25 seconds) where twilio may or may not complete a transactional request and we'd be in better shape if twiio made that decision.

I think that having a timeout and transactional_timeout with sane defaults built in by Librarian would go a good way, even if they're opt-in.

Meanwhile, I've adjusted http timeouts in our workers to 28 seconds and kept it at 10 seconds for web dynos, though it's probably reasonable to reduce that to 2 seconds even

I can propose this in the twilio-ruby library, but probably it should be client-wide or at least in the docs, so that'd be Librarian, right? ref #493

I'll consider this closed as letting twilio close transactional requests seems to be the way to go.

My support rep responded to this with

I would just recommend making your timeout suggestions in the twilio-ruby library:

https://github.com/twilio/twilio-ruby/issues

so, ¯\_(ツ)_/¯ here's where I am

@bf4
Copy link
Author

bf4 commented Jul 1, 2021

related? #559

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: easy fix is easy in difficulty status: help wanted requesting help from the community type: community enhancement feature request not on Twilio's roadmap type: docs update documentation change not affecting the code
Projects
None yet
Development

No branches or pull requests

3 participants