Use the open_timeout kwarg where available#95
Use the open_timeout kwarg where available#95mohamedhafez wants to merge 1 commit intoruby:masterfrom
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.
@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.
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.
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.
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.
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: