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

Breaking change to Rack::Timeout causes Heroku deployment failure #918

Closed
orenyk opened this issue Jun 5, 2018 · 4 comments
Closed

Breaking change to Rack::Timeout causes Heroku deployment failure #918

orenyk opened this issue Jun 5, 2018 · 4 comments

Comments

@orenyk
Copy link
Contributor

orenyk commented Jun 5, 2018

See here for more info. It looks like we can just update this file - I can try to open up a PR later tonight. Thanks!

@orenyk
Copy link
Contributor Author

orenyk commented Jun 6, 2018

Based on the README it seems like the recommended way to modify the timeout is now through environment variables. Two questions:

  1. Would adding the environment variable (RACK_TIMEOUT_SERVICE_TIMEOUT) to the .env template with a default value of 10 be sufficient to match the current Suspenders functionality?
  2. Out of curiosity, is there a reason why you prefer 10 seconds to the default of 15?

@composerinteralia
Copy link
Contributor

Hey Oren, nice to see you again (we met at the last RailsConf). I would be happy to help you get this through. Are you ready to make a PR, or would you like any help?

@toobulkeh
Copy link
Contributor

toobulkeh commented Jun 28, 2018

I would suggest just using the new defaults for Rack::Timeout, as the new config would require all 4 variables to be set to 10, since Heroku just broke them out. I.E. just delete the entire custom RackTimeout config:

https://github.com/heroku/rack-timeout/blob/master/README.md#configuring

Rack::Timeout takes the following settings, shown here with their default values and associated environment variables.

service_timeout:   15     # RACK_TIMEOUT_SERVICE_TIMEOUT
wait_timeout:      30     # RACK_TIMEOUT_WAIT_TIMEOUT
wait_overtime:     60     # RACK_TIMEOUT_WAIT_OVERTIME
service_past_wait: false  # RACK_TIMEOUT_SERVICE_PAST_WAIT

These settings can be overriden during middleware initialization or environment variables RACK_TIMEOUT_* mentioned above. Middleware parameters take precedence:

use Rack::Timeout, service_timeout: 5, wait_timeout: false

@orenyk
Copy link
Contributor Author

orenyk commented Jul 5, 2018

@composerinteralia great to hear from you and congrats on the new gig 😄! I'm happy to open a PR - I just wanted to make sure my approach above made sense.

To @toobulkeh's point, it looks like the old Rack::Timeout.timeout= method set the service_timeout attribute, so that was the only thing changed in the old Suspenders config and we can easily replicate that in our .env file.

I'll throw a PR up and we can move this discussion there. Thanks!

orenyk added a commit to orenyk/suspenders that referenced this issue Jul 5, 2018
Resolves thoughtbot#918

Rack::Timeout v0.5.0 removed legacy setter methods used for
configuration (zombocom/rack-timeout#125) such
that the configuration added by Suspenders causes a NoMethodError when
initializing Rails in the production environment (e.g. during Heroku
deployment). This PR switches to using environment variable-based
configuration of Rack::Timeout, matching the recommendations in the
README.
composerinteralia pushed a commit that referenced this issue Jul 21, 2018
Resolves #918

Rack::Timeout v0.5.0 removed legacy setter methods used for
configuration (zombocom/rack-timeout#125) such
that the configuration added by Suspenders causes a NoMethodError when
initializing Rails in the production environment (e.g. during Heroku
deployment). This PR switches to using environment variable-based
configuration of Rack::Timeout, matching the recommendations in the
README.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants