-
Notifications
You must be signed in to change notification settings - Fork 35
Use the open_timeout kwarg where available #95
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
base: master
Are you sure you want to change the base?
Conversation
463e261 to
4ac4936
Compare
|
Looks nice! It could be worth doing something like ruby/net-http#250 to avoid nested exceptions? |
4ac4936 to
64e8205
Compare
|
@osyoyu done! |
lib/net/smtp.rb
Outdated
| rescue Errno::ETIMEDOUT => e | ||
| raise Net::OpenTimeout.new(e) # for compatibility with previous versions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@osyoyu i also moved this rescue block here to be more a bit more closer to where its thrown and in my opinion a bit easier to understand. if you agree maybe you could do it in net-http as well:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless the following code is also able to throw an Errno::ETIMEDOUT? Like what would happen if @open_timeout was set to like 5 hours or something, eventually the OS would time it out and throw some kind of error, right?
Timeout.timeout(@open_timeout, Net::OpenTimeout) do
tcp_socket(@address, @port)
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually that seems to throw a IO::TimeoutError, at least on Ruby 3.4, so i do think moving this rescue block does provide a bit more clarity of where the Errno::ETIMEDOUT is coming from, to whoever comes by next to edit this code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat, but in the net-http case, won't this lead to diverged messages between timeout impls? Failed to open TCP connection to ... is set in the re-raise in rescue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AH! correct in the case of net-http. There doesn't seem to be any messing with the error messages in net-smtp though so i think this little change makes sense but only for this gem
64e8205 to
dd4205c
Compare
Copies what was done in releases 0.9.0 and 0.9.1 of net-http
dd4205c to
ef6c6ba
Compare
|
Made the same changes here as in net-http 0.9.0 and 0.9.1, all tests passing |
Resolves #94
equivalent change to what @osyoyu did in ruby/net-http#224
Some justification copied over from that PR's description: