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

Allow setting write_timeout for connections on Ruby 2.6+ #950

Merged
merged 1 commit into from Oct 14, 2020

Conversation

bdewater
Copy link
Contributor

@bdewater bdewater commented Oct 6, 2020

See original Ruby issue: https://bugs.ruby-lang.org/issues/13396 and implementation: ruby/ruby@bd7c46a

In Ruby 2.6+ this defaults to 60s. I picked 30 seconds to make it in line with open_timeout. IMO the defaults are too generous but that's a discussion for another time :)

@CLAassistant
Copy link

CLAassistant commented Oct 6, 2020

CLA assistant check
All committers have signed the CLA.

@brandur-stripe
Copy link
Contributor

Thanks @bdewater! This looks good to me. Do you think that in StripeConfiguration#write_timeout= we could error if someone tries to set it in Ruby < 2.6? It doesn't seem like a good idea to make it settable but have it do nothing in case people are not on 2.6 yet, and most users probably aren't.

IMO the defaults are too generous but that's a discussion for another time :)

Hah, +1, but yep, let's defer that conversation.

@bdewater
Copy link
Contributor Author

Feedback addressed and CLA signed.

Copy link
Contributor

@brandur-stripe brandur-stripe left a comment

Choose a reason for hiding this comment

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

Thanks! This LGTM.

@brandur-stripe brandur-stripe merged commit 085e081 into stripe:master Oct 14, 2020
@brandur-stripe
Copy link
Contributor

Released as 5.28.0.

@bdewater bdewater deleted the write-timeout branch October 14, 2020 18:46
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

Successfully merging this pull request may close these issues.

None yet

3 participants