Neither client bounds a read. A server that accepts the connection and then says nothing leaves the caller blocked with no way out -- the socket is open, the read simply never returns, and nothing notices.
This is not hypothetical. Pointing a STARTTLS session at an implicit-TLS port produces exactly that: the client waits for a greeting, the server waits for a ClientHello. In the test suite it cost 180 seconds per occurrence, and only because dovecot eventually closed the socket at its own login timeout. Against a server without one it would have waited indefinitely.
Measured, in the container:
| test |
before |
after |
| net_pop3_live_test |
181.4s |
3.4s |
| net_imap_live_test |
187.1s |
9.5s |
#166 adds set_timeout() to both clients and applies it before the greeting is read. Everything it needed already existed -- basic_socketstream::set_timeout(), inherited by every stream type these use -- the clients just never called it.
What that branch deliberately did not do is change the default, which is still zero, meaning forever. Changing it changes behaviour for every existing caller, and that is a decision rather than a fix.
The case for changing it: a network client whose default is to hang is a footgun, and every caller that has not thought about it has the bug. The case against: a long-running fetch over a slow link is a legitimate thing to want, and a default deadline turns it into a failure.
A middle option is a default that is generous rather than absent -- a few minutes, say -- so that a wedged connection eventually reports something instead of nothing, while an honest slow transfer still completes. Whichever way it goes, it should be written down in the header, because the current behaviour is only discoverable by waiting.
Neither client bounds a read. A server that accepts the connection and then says nothing leaves the caller blocked with no way out -- the socket is open, the read simply never returns, and nothing notices.
This is not hypothetical. Pointing a STARTTLS session at an implicit-TLS port produces exactly that: the client waits for a greeting, the server waits for a ClientHello. In the test suite it cost 180 seconds per occurrence, and only because dovecot eventually closed the socket at its own login timeout. Against a server without one it would have waited indefinitely.
Measured, in the container:
#166 adds
set_timeout()to both clients and applies it before the greeting is read. Everything it needed already existed --basic_socketstream::set_timeout(), inherited by every stream type these use -- the clients just never called it.What that branch deliberately did not do is change the default, which is still zero, meaning forever. Changing it changes behaviour for every existing caller, and that is a decision rather than a fix.
The case for changing it: a network client whose default is to hang is a footgun, and every caller that has not thought about it has the bug. The case against: a long-running fetch over a slow link is a legitimate thing to want, and a default deadline turns it into a failure.
A middle option is a default that is generous rather than absent -- a few minutes, say -- so that a wedged connection eventually reports something instead of nothing, while an honest slow transfer still completes. Whichever way it goes, it should be written down in the header, because the current behaviour is only discoverable by waiting.