Revert "Update mio to 0.8 (#4270)" and dependent changes #4392
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
It seems that cc8ad36 also needs to be reverted to fix remaining regression (double panic from hyper's test suite) that is currently occurring on master. |
|
Hmmm, alright, I will do that in a separate PR. |
taiki-e
approved these changes
Jan 11, 2022
This can be reproduced by running the following commands in the tokio's workspace root: git clone https://github.com/hyperium/hyper.git
cd hyper
echo '[workspace]' >>Cargo.toml
echo '[patch.crates-io]' >>Cargo.toml
echo 'tokio = { path = "../tokio" }' >>Cargo.toml
echo 'tokio-util = { path = "../tokio-util" }' >>Cargo.toml
echo 'tokio-stream = { path = "../tokio-stream" }' >>Cargo.toml
echo 'tokio-test = { path = "../tokio-test" }' >>Cargo.toml
cargo test --features full --test integrationresult: https://github.com/tokio-rs/tokio/runs/4779296976?check_suite_focus=true |
mxinden
added a commit
to mxinden/rust-libp2p
that referenced
this pull request
Jan 29, 2022
Within `Provider::new_stream` we wait for the socket to become writable (`stream.writable`), before returning it as a stream. In other words, we are waiting for the socket to connect before returning it as a new TCP connection. Waiting to connect before returning it as a new TCP connection allows us to catch TCP connection establishment errors early. While `stream.writable` drives the process of connecting, it does not surface potential connection errors themselves. These need to be explicitly collected via `TcpSocket::take_error`. If not explicitly collected, they will surface on future operations on the socket. For now this commit explicitly calls `TcpSocket::take_error` when using `async-io` only. `tokio` introduced the method (`take_error`) in tokio-rs/tokio#4364 though later reverted it in tokio-rs/tokio#4392. Once re-reverted, the same patch can be applied when using `libp2p-tcp` with tokio. --- One example on how this bug surfaces today: A `/dnsaddr/xxx` `Multiaddr` can potentially resolve to multiple IP addresses, e.g. to the IPv4 and the IPv6 addresses of a node. `libp2p-dns` tries dialing each of them in sequence using `libp2p-tcp`, returning the first that `libp2p-tcp` reports as successful. Say that the local node tries the IPv6 address first. In the scenario where the local node's networking stack does not support IPv6, e.g. has no IPv6 route, the connection attempt to the resolved IPv6 address of the remote node fails. Given that `libp2p-tcp` does not call `TcpSocket::take_error`, it would falsly report the TCP connection attempt as successful. `libp2p-dns` would receive the "successful" TCP connection for the IPv6 address from `libp2p-tcp` and would not attempt to dial the IPv4 address, even though it supports IPv4, and instead bubble up the "successful" IPv6 TCP connection. Only later, when writing or reading from the "successful" IPv6 TCP connection, would the IPv6 error surface.
mxinden
added a commit
to libp2p/rust-libp2p
that referenced
this pull request
Feb 1, 2022
Within `Provider::new_stream` we wait for the socket to become writable (`stream.writable`), before returning it as a stream. In other words, we are waiting for the socket to connect before returning it as a new TCP connection. Waiting to connect before returning it as a new TCP connection allows us to catch TCP connection establishment errors early. While `stream.writable` drives the process of connecting, it does not surface potential connection errors themselves. These need to be explicitly collected via `TcpSocket::take_error`. If not explicitly collected, they will surface on future operations on the socket. For now this commit explicitly calls `TcpSocket::take_error` when using `async-io` only. `tokio` introduced the method (`take_error`) in tokio-rs/tokio#4364 though later reverted it in tokio-rs/tokio#4392. Once re-reverted, the same patch can be applied when using `libp2p-tcp` with tokio. --- One example on how this bug surfaces today: A `/dnsaddr/xxx` `Multiaddr` can potentially resolve to multiple IP addresses, e.g. to the IPv4 and the IPv6 addresses of a node. `libp2p-dns` tries dialing each of them in sequence using `libp2p-tcp`, returning the first that `libp2p-tcp` reports as successful. Say that the local node tries the IPv6 address first. In the scenario where the local node's networking stack does not support IPv6, e.g. has no IPv6 route, the connection attempt to the resolved IPv6 address of the remote node fails. Given that `libp2p-tcp` does not call `TcpSocket::take_error`, it would falsly report the TCP connection attempt as successful. `libp2p-dns` would receive the "successful" TCP connection for the IPv6 address from `libp2p-tcp` and would not attempt to dial the IPv4 address, even though it supports IPv4, and instead bubble up the "successful" IPv6 TCP connection. Only later, when writing or reading from the "successful" IPv6 TCP connection, would the IPv6 error surface. Co-authored-by: Oliver Wangler <[email protected]>
|
👋 Do you plan to re-introduce Thanks for all the help! |
|
Yeah, we should go through all of these and reintroduce them. |
santos227
added a commit
to santos227/rustlib
that referenced
this pull request
Jun 20, 2022
Within `Provider::new_stream` we wait for the socket to become writable (`stream.writable`), before returning it as a stream. In other words, we are waiting for the socket to connect before returning it as a new TCP connection. Waiting to connect before returning it as a new TCP connection allows us to catch TCP connection establishment errors early. While `stream.writable` drives the process of connecting, it does not surface potential connection errors themselves. These need to be explicitly collected via `TcpSocket::take_error`. If not explicitly collected, they will surface on future operations on the socket. For now this commit explicitly calls `TcpSocket::take_error` when using `async-io` only. `tokio` introduced the method (`take_error`) in tokio-rs/tokio#4364 though later reverted it in tokio-rs/tokio#4392. Once re-reverted, the same patch can be applied when using `libp2p-tcp` with tokio. --- One example on how this bug surfaces today: A `/dnsaddr/xxx` `Multiaddr` can potentially resolve to multiple IP addresses, e.g. to the IPv4 and the IPv6 addresses of a node. `libp2p-dns` tries dialing each of them in sequence using `libp2p-tcp`, returning the first that `libp2p-tcp` reports as successful. Say that the local node tries the IPv6 address first. In the scenario where the local node's networking stack does not support IPv6, e.g. has no IPv6 route, the connection attempt to the resolved IPv6 address of the remote node fails. Given that `libp2p-tcp` does not call `TcpSocket::take_error`, it would falsly report the TCP connection attempt as successful. `libp2p-dns` would receive the "successful" TCP connection for the IPv6 address from `libp2p-tcp` and would not attempt to dial the IPv4 address, even though it supports IPv4, and instead bubble up the "successful" IPv6 TCP connection. Only later, when writing or reading from the "successful" IPv6 TCP connection, would the IPv6 error surface. Co-authored-by: Oliver Wangler <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The upgrade to Mio 0.8 came with other changes that introduced a regression
not caught by our CI. While we could move forward and fix that change, in an
effort to keep master "releasable" at all times, I propose reverting the changes
to give us time to re-introduce them after we fix the CI changes.
PR #4390 includes the fix + improving CI, but cannot be merged yet because
the CI improvements don't pass yet.
This reverts commits: