Skip to content

Conversation

@mohamedhafez
Copy link
Contributor

@mohamedhafez mohamedhafez commented Nov 10, 2025

Resolves #94

equivalent change to what @osyoyu did in ruby/net-http#224

Some justification copied over from that PR's description:

The builtin timeout in TCPSocket.open is better in several ways. First, it does not rely on a separate Ruby Thread for monitoring Timeout (which is what the timeout library internally does). Also, it is compatible with Ractors, since it does not rely on Mutexes (which is also what the timeout library does).

@osyoyu
Copy link

osyoyu commented Dec 4, 2025

Looks nice! It could be worth doing something like ruby/net-http#250 to avoid nested exceptions?

@mohamedhafez
Copy link
Contributor Author

@osyoyu done!

lib/net/smtp.rb Outdated
Comment on lines 680 to 681
rescue Errno::ETIMEDOUT => e
raise Net::OpenTimeout.new(e) # for compatibility with previous versions
Copy link
Contributor Author

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:)

Copy link
Contributor Author

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

Copy link
Contributor Author

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

Copy link

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

Copy link
Contributor Author

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

Copies what was done in releases 0.9.0 and 0.9.1 of net-http
@mohamedhafez
Copy link
Contributor Author

Made the same changes here as in net-http 0.9.0 and 0.9.1, all tests passing

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.

Replace Timeout.timeout with TCPSocket.open(open_timeout:) when available

2 participants