Remove TcpStream::peer_addr check for network sockets, test epoll readiness for shutdown#5009
Remove TcpStream::peer_addr check for network sockets, test epoll readiness for shutdown#5009WhySoBad wants to merge 3 commits intorust-lang:masterfrom
TcpStream::peer_addr check for network sockets, test epoll readiness for shutdown#5009Conversation
|
Thank you for contributing to Miri! A reviewer will take a look at your PR, typically within a week or two. |
| // Closing the read end of a socket causes an EPOLLRDHUP event. | ||
| readiness.read_closed |= is_read_shutdown || is_read_write_shutdown; | ||
| // Only shutting down the write end doesn't cause an EPOLLHUP event | ||
| // and thus we won't set the `write_closed` readiness for it here. | ||
| readiness.write_closed |= is_read_write_shutdown; |
There was a problem hiding this comment.
Have you confirmed that this matches what Linux does?
There was a problem hiding this comment.
Yes, it's pretty much the exact same as the kernel does:
https://github.com/torvalds/linux/blob/aa54b1d27fe0c2b78e664a34fd0fdf7cd1960d71/net/ipv4/tcp.c#L584-L588
There was a problem hiding this comment.
(and the tests also pass on my Linux host)
There was a problem hiding this comment.
The kernel seems to also set EPOLLIN?
| // Because we map cross platform mio readiness to epoll readiness and | ||
| // the different platforms don't treat `shutdown` the same way, we set | ||
| // the readiness after a `shutdown` manually to achieve more consistent | ||
| // epoll readiness. |
There was a problem hiding this comment.
What goes wrong (on which hosts) if we don't do this?
There was a problem hiding this comment.
On Windows hosts the tests which only shut down the read/write end of the socket hang indefinitely otherwise.
There was a problem hiding this comment.
| // Because we map cross platform mio readiness to epoll readiness and | |
| // the different platforms don't treat `shutdown` the same way, we set | |
| // the readiness after a `shutdown` manually to achieve more consistent | |
| // epoll readiness. | |
| // Because we map cross platform mio readiness to epoll readiness and | |
| // the different platforms don't treat `shutdown` the same way, we set | |
| // the readiness after a `shutdown` manually to achieve more consistent | |
| // epoll readiness. Otherwise we do not generate enough epoll events | |
| // on partial shutdowns on Windows hosts. |
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
There was a problem hiding this comment.
The bug occurs when we connect a non-blocking socket and shut down the write end of the connecting socket before the socket became connected. As a consequence, the TcpStream::peer_addr call in the ensure_connected method fails because after closing the write end, we can no longer get the peer. Our current logic treats this failure as an OS error and panics.
Is it possible to add / extend a test to cover that case?
There was a problem hiding this comment.
We cannot really test this because we need a socket which is stuck at connecting to a address + port and to which address + port we later bind a listener. Because we need to use randomized ports to avoid "address is already used" errors, we cannot know such a port before binding (listening to get a TcpListener).
There was a problem hiding this comment.
We could pick something like 127.0.0.208:23754 that is unlikely to be used.
|
@rustbot ready |
rustbot
added
S-waiting-on-review
| // [`TcpStream::take_err`] and [`TcpStream::peer_addr`] both | ||
| // don't return an error after receiving an [`Interest::WRITEABLE`] | ||
| // event on the stream. | ||
| // Mio does not wait until the stream is fully connected. |
There was a problem hiding this comment.
On second thought, here's a better wording:
| // Mio does not wait until the stream is fully connected. | |
| // This begins establishing the connection, but does not block until the stream is fully connected. |
|
@rustbot author |
rustbot
added
S-waiting-on-author
3 participants
As mentioned in #5005, this pull request fixes a bug with
shutdownand non-blocking sockets. Additionally, it also adds tests forshutdownin combination withepoll.The bug occurs when we connect a non-blocking socket and shut down the write end of the connecting socket before the socket became connected. As a consequence, the
TcpStream::peer_addrcall in theensure_connectedmethod fails because after closing the write end, we can no longer get the peer. Our current logic treats this failure as an OS error and panics.Because tokio doesn't implement this suggested check with
TcpStream::peer_addrand everything seems to work fine, we should also be fine with removing the check and by this resolving the panic.