fix async protocol.read().await not returning on closed sockets #160
Conversation
When read() returns with zero bytes read, this indicates an EOF. Previously, we'd continue to read() in a loop and never return to the caller if that happend. Fix this by returning a ProtocolError with ErrorKind::ConnectionAborted "read zero bytes". This results in an ProtocolFailureSuggestion::Abort. fixes rust-bitcoin#159
| let len = self.reader.read(&mut length_bytes[*bytes_read..]).await?; | ||
| if len == 0 { | ||
| return Err(std::io::Error::new( | ||
| std::io::ErrorKind::ConnectionAborted, |
There was a problem hiding this comment.
Went with ConnectionAborted here. ConnectionReset is an alternative. Happy to be convinced to use ConnectionReset if someone has strong feelings about it.
There was a problem hiding this comment.
No strong opinion here, ConnectionAborted is fine with me.
There was a problem hiding this comment.
Should we use UnexpectedEof? It looks like that is what tokio bubbles up in similar scenarios.
There was a problem hiding this comment.
I thought about this too, but found the Rust stdlib docs to not to be a perfect fit for our case? https://doc.rust-lang.org/std/io/enum.ErrorKind.html#variant.UnexpectedEof
An error returned when an operation could not be completed because an “end of file” was reached prematurely.
This typically means that an operation could only succeed if it read a particular number of bytes but only a smaller number of bytes could be read.
(but also don't really care too much personally)
| println!( | ||
| "Trying to read another message from the server, while the connection is already closed." | ||
| ); | ||
| assert!(protocol.read().await.is_err()); |
There was a problem hiding this comment.
Hm the test still passed when async writing to the closed protocol. I think that should return an error too? At least it doesn't hang..
index 5b96eca..e7cb5d1 100644
--- a/protocol/tests/round_trips.rs
+++ b/protocol/tests/round_trips.rs
@@ -229,6 +229,10 @@ async fn pingpong_with_closed_connection_async() {
"Trying to read another message from the server, while the connection is already closed."
);
assert!(protocol.read().await.is_err());
+
+ let ping = V2NetworkMessage::new(NetworkMessage::Ping(45324));
+ let message = consensus::serialize(&ping);
+ protocol.write(&Payload::genuine(message)).await.unwrap();
}
#[test]Probably a good followup once this is merged.
|
From the
It appears we are receiving said 0 and then slamming the CPU in the cc @nyonson |
I'd like to keep the functions cancellation safe if possible (read_exact isn't so would break that). |
|
That's right. In that case I think this approach works. Haven't review the test yet. |
|
@0xB10C thanks for the report + fix! Hoping to give this library some more attention once we wrap up the consensus_encoding work in rust-bitcoin. |
This adds a test for the bug described in rust-bitcoin#159 and fixed in the previous commit. The test starts a ping-pong server that only respondes to one Ping(x) with a Pong(x) and then stops. Further async protocol read()'s should now error. Addtionally, also test that writing a message to closed protocol fails.
0xB10C
force-pushed
the
2026-01-fix-reads-on-closed-socket
branch
from
4019a67 to
0719bcc
Compare
fwiw I think these are the relevant docs for the read from
(this sounds like |
|
LGTM |
In the sync code, read() seems to work. I'll PR a test for it. However, write() on a closed socket seems to not error too as in #161 |
This implements the sync variant of 0719bcc. While this works at the moment, I think it's good to have this test for extra coverage. It can also be easily extended in a follow up to test sync write() on a closed socket (see rust-bitcoin#160 (comment)).
This implements the sync variant of 0719bcc. While this works at the moment, I think it's good to have this test for extra coverage. It can also be easily extended in a follow up to test sync write() on a closed socket (see rust-bitcoin#160 (comment)).
3 participants
The first commit fixes #159 and the second adds a test. To test the fix, revert the first commit and run the test:
cargo test --test round_trips pingpong_with_closed_connection_async --features tokioI haven't tested if the sync code works. Would probably be good to extend the tests for this too.