net: fix TcpSocket bugs introduced in #4270 #4390
Conversation
|
Oh, timeout in hyper/benches/connect.rs doesn't seem to happen on linux?: https://github.com/tokio-rs/tokio/runs/4775733526?check_suite_focus=true EDIT: It seems to be unrelated to #4270 as it can be reproduced in tokio v1.15.0 on macos. |
tokio/src/net/tcp/socket.rs
Outdated
| @@ -125,6 +125,7 @@ impl TcpSocket { | |||
| socket2::Type::STREAM, | |||
| Some(socket2::Protocol::TCP), | |||
| )?; | |||
| inner.set_nonblocking(true)?; | |||
There was a problem hiding this comment.
There should be a way to do this as part of the Socket::new initialization. If there isn't, we need to fix that.
There was a problem hiding this comment.
I'm thinking we should revert the changes that got us here until this is fixed (unless there is a quick fix).
There was a problem hiding this comment.
Socket2 supports Type::nonblocking for this.
tokio/src/net/tcp/socket.rs
Outdated
| @@ -125,6 +125,7 @@ impl TcpSocket { | |||
| socket2::Type::STREAM, | |||
| Some(socket2::Protocol::TCP), | |||
| )?; | |||
| inner.set_nonblocking(true)?; | |||
There was a problem hiding this comment.
Socket2 supports Type::nonblocking for this.
| match self.inner.connect(&addr.into()) { | ||
| // https://github.com/tokio-rs/mio/blob/d400ddfc97212b4a2844d741b095dab2c6d15543/src/sys/unix/tcp.rs#L31 | ||
| #[cfg(unix)] | ||
| Err(e) if e.raw_os_error() != Some(libc::EINPROGRESS) => return Err(e), |
There was a problem hiding this comment.
@carllerche @seanmonstar I think this should fix the issue.
|
The double panic from hyper's test suite also seems to be unrelated to this problem. I can confirm the double panic on #4391. |
taiki-e
changed the title
This reverts commit cc8ad36. That commit caused double panic in hyper's test suite.
|
Ok, reverting cc8ad36 solves the remaining problems. |
|
I can try debugging this tonight. I have access to MacOS and Windows machines for development. |
|
|
||
| test-hyper: | ||
| name: Test hyper | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
Could we run hyper tests on other platforms as well?
There was a problem hiding this comment.
#4393 (CI part of this PR) contains changes for that.
|
I'm going to close this PR since the changes being fixed were reverted and the CI improvements were merged separately. We need to figure out how we want to move forward w/ mio 0.8 still. |
This is a rather surprising reaction, especially considering the bug wasn't even in Mio. Really the only problem was the missing check for |
This is an attempt to fix bugs of TcpSocket that I accidentally introduced in #4270.
This should pass tokio's test suite, but tokio has very few tests for TcpSocket (this patch adds a few simple assertions), so we need to run hyper's test suite to see if it works correctly.
This patch is still incomplete, and the following twoone hyper's tests are currently failing.UPD: Timeout in hyper/benches/connect.rs seems to be unrelated to #4270 as it can be reproduced in tokio v1.15.0.
UPD2: Double panic in hyper/tests/integration.rs is due to #4377
I have very little bandwidth on the weekdays, so if anyone knows how to fix these issues, feel free to copy this patch and open a new PR.