Prefer close() to shutdown(), it waits for finish #98

carver · 2023-07-27T23:56:54Z

stream.close().await will not return until the connection exits, either from an error, or from all data being written and ACKed.

To simplify, shutdown() is no longer part of the public API.

jacobkaufmann

took an initial pass. need to look more at the mock stuff, but I think it's in the right general direction!

src/udp.rs

tests/stream.rs

src/udp.rs

src/lib.rs

src/stream.rs

jacobkaufmann · 2023-08-03T15:00:48Z

src/testutils.rs

+        let result = self.outbound.send(buf.to_vec());
+        if result.is_err() {
+            Err(io::Error::new(
+                io::ErrorKind::UnexpectedEof,
+                "channel closed: {result:?}",


Suggested change

let result = self.outbound.send(buf.to_vec());

if result.is_err() {

Err(io::Error::new(

io::ErrorKind::UnexpectedEof,

"channel closed: {result:?}",

if let Err(err) = self.outbound.send(buf.to_vec()) {

Err(io::Error::new(

io::ErrorKind::UnexpectedEof,

"channel closed: {err}",

src/testutils.rs

jacobkaufmann

I think that once we resolve these comments we will be ready to merge

jacobkaufmann · 2023-08-05T00:04:24Z

tests/stream.rs

+
+// Test that close() returns a timeout, if recipient is not ACKing (after a successful connection)
+#[tokio::test]
+async fn that_close_errors_if_all_packets_dropped() {


Suggested change

async fn that_close_errors_if_all_packets_dropped() {

async fn close_errors_if_all_packets_dropped() {

jacobkaufmann · 2023-08-05T00:11:35Z

src/testutils.rs

+    socket_a: &MockUdpSocket,
+    socket_b: &MockUdpSocket,


there's nothing here that enforces that A's only peer is B and vice versa. if that doesn't hold, then the connection ID pair doesn't seem to be a "pair"

The code that this comment is referring to is intended to enforce the pair: https://github.com/ethereum/utp/pull/98/files#r1285253920

So I think "pair" still works.

see comment above, but I think it's better to give this back to the caller along with the socket upon creation of the link

jacobkaufmann · 2023-08-06T19:13:29Z

src/stream.rs

@@ -101,10 +104,21 @@ where
            Err(err) => Err(io::Error::new(io::ErrorKind::Other, format!("{err:?}"))),
        }
    }
+
+    /// Close this connection, after finishing the pending writes


nit:

Suggested change

/// Close this connection, after finishing the pending writes

/// Closes the stream gracefully.

/// Completes when the remote peer acknowledges all sent data.

src/testutils.rs

jacobkaufmann · 2023-08-06T19:23:55Z

src/testutils.rs

+        let result = self.outbound.send(buf.to_vec());
+        if let Err(err) = result {


Suggested change

let result = self.outbound.send(buf.to_vec());

if let Err(err) = result {

if let Err(err) = self.outbound.send(buf.to_vec()) {

jacobkaufmann · 2023-08-06T19:28:44Z

src/testutils.rs

+    pub only_peer: char,
+    /// Watch for signal that the link is up. If not up, link will SILENTLY drop all sent packets.
+    /// If up_status is None, the link is always up.
+    up_status: Option<watch::Receiver<bool>>,


why not use something like an AtomicBool here? it seems unnecessary to have to worry about the None variant here, and that we just want a "switch" to open and close the link.

jacobkaufmann · 2023-08-06T19:40:17Z

tests/stream.rs

+// After adding some mechanism that sends a RESET packet, add the following test:
+// Test that close() returns ConnectionReset if recipient resets during transfer


let's move this to an issue

👍🏻 #102

src/udp.rs

jacobkaufmann · 2023-08-06T19:59:21Z

src/testutils.rs

+pub fn build_link_pair() -> (MockUdpSocket, MockUdpSocket) {
+    let (peer_a, peer_b): (char, char) = ('A', 'B');


nit: I would prefer something more in line with the (tx, rx) = channel() semantics

Suggested change

pub fn build_link_pair() -> (MockUdpSocket, MockUdpSocket) {

let (peer_a, peer_b): (char, char) = ('A', 'B');

pub fn mock_link((a, b): (char, char)) -> (MockUdpSocket, MockUdpSocket) {

and ideally the connection ID stuff would also be taken care of here, since there is only ever one connection per socket. if that can't be done easily here, then we can come back to it, but I think it would be nice to unify the two steps to enforce consistency.

nit: I would prefer something more in line with the (tx, rx) = channel() semantics

Hm, are you talking about the refactor I mentioned in #100 ?

one natural path is to split the socket into a sender and receiver (much how creating a channel returns a split tx and rx object)

That refactor is interesting, but feels significant and out of scope. I'm not really sure what it would look like without changing how the socket trait is defined.

and ideally the connection ID stuff would also be taken care of here, since there is only ever one connection per socket. if that can't be done easily here, then we can come back to it, but I think it would be nice to unify the two steps to enforce consistency.

You're thinking like a single method that calls both build_link_pair() and build_connection_id_pair() and then returns the 4 resulting values?

the last bit is what I'm referring to yes, returning a pair of pairs. and then those other functions don't need to be pub.

jacobkaufmann · 2023-08-06T20:06:03Z

src/testutils.rs

+            .await
+            .ok_or_else(|| io::Error::new(io::ErrorKind::UnexpectedEof, "channel closed"))?;
+        if buf.len() < packet.len() {
+            panic!("buffer too small for perfect link");


do we need to panic here?

So the other options I see:

Return n=0, which gives the tester a warning log "unable to decode uTP packet"

Put the first part of the packet, up to the buffer size, into the buffer. again, giving an "unable to decode uTP packet"

Return an io::Error, maybe an InvalidInput. Note that src/socket seems to drop errors on the floor without a log

Any other suggestions?

Since this is a utility aimed at testing instead of production, panicking with a clear reason is more important than staying alive. None of these other options seem to give the tester much information about why it failed, and some will fail quietly, which means a long timeout/retry loop that's hard to debug.

So I don't see a better option than panicking, yet.

fair enough. I'm okay with keeping the panic, but we should document it as we did above for send_to.

carver · 2023-08-08T00:18:28Z

Ok, I've finished this round of changes. The only change here that isn't an implementation from your feedback is 0d2a6da

I wanted a way to test the close() timeout completely, but did not want to wait the whole wall-clock time for the test to complete. Luckily, I found tokio's pause, which handles it beautifully. (even though I find it awkwardly named)

Close errors will be used more, when ethereum#98 lands, which returns the close error to clients who wait for the stream to exit.

stream.close().await will not return until the connection exits, either from an error, or from all data being written and ACKed.

carver · 2023-08-08T17:11:13Z

Just rebased on #101

jacobkaufmann

just a few minor remaining things. up to you whether to address the nits.

jacobkaufmann · 2023-08-08T20:23:08Z

src/testutils.rs

+            .await
+            .ok_or_else(|| io::Error::new(io::ErrorKind::UnexpectedEof, "channel closed"))?;
+        if buf.len() < packet.len() {
+            panic!("buffer too small for perfect link");


fair enough. I'm okay with keeping the panic, but we should document it as we did above for send_to.

jacobkaufmann · 2023-08-08T20:26:18Z

src/testutils.rs

+pub fn build_link_pair() -> (MockUdpSocket, MockUdpSocket) {
+    let (peer_a, peer_b): (char, char) = ('A', 'B');


the last bit is what I'm referring to yes, returning a pair of pairs. and then those other functions don't need to be pub.

jacobkaufmann · 2023-08-08T20:28:24Z

src/testutils.rs

+    socket_a: &MockUdpSocket,
+    socket_b: &MockUdpSocket,


see comment above, but I think it's better to give this back to the caller along with the socket upon creation of the link

jacobkaufmann · 2023-08-08T20:30:39Z

src/testutils.rs

+impl MockUdpSocket {
+    /// Grab a status controller for this socket.
+    /// Set the status to false to simulate a down link.
+    pub fn get_status_controller(&mut self) -> Arc<AtomicBool> {


nit:

Suggested change

pub fn get_status_controller(&mut self) -> Arc<AtomicBool> {

pub fn up_status(&mut self) -> Arc<AtomicBool> {

jacobkaufmann · 2023-08-08T20:31:29Z

src/testutils.rs

+    }
+
+    /// Should the link send packets?
+    fn active(&self) -> bool {


nit:

Suggested change

fn active(&self) -> bool {

fn is_active(&self) -> bool {

Use the new mock socket to: - remove chance of real dropped packets on UDP during test - allow parallel tests without fiddling with UDP ports - convenience method to launch connected peers - test what happens when network goes down Add test that close() waits for writes to ACK and eventually returns a timeout, by disabling the network after connecting to a peer. Closing the connection will time out, because the sender is waiting for ACKs that the recipient will never send. (Because the recipient never recieves the data) Why does this add the tokio test-util and start_paused = true? We don't want the test to actually wait the timeout length while running, so we use tokio's pause feature to skip over the timeout without interrupting anything else that would happen in real-time: https://docs.rs/tokio/latest/tokio/time/fn.pause.html

carver · 2023-08-09T01:40:51Z

Thanks, that's a wrap!

Close errors will be used more, when ethereum#98 lands, which returns the close error to clients who wait for the stream to exit.

carver requested a review from jacobkaufmann July 31, 2023 02:42

carver self-assigned this Jul 31, 2023

jacobkaufmann reviewed Aug 3, 2023

View reviewed changes

carver force-pushed the blocking-close-api branch from 659adf4 to 159d80d Compare August 3, 2023 17:58

carver requested a review from jacobkaufmann August 3, 2023 17:59

jacobkaufmann reviewed Aug 6, 2023

View reviewed changes

This was referenced Aug 7, 2023

require &mut self in AsyncUdpSocket methods #101

Merged

Test that close() exits when peer sends a RESET packet #102

Open

carver force-pushed the blocking-close-api branch from 159d80d to 14ddfd0 Compare August 8, 2023 00:10

carver requested a review from jacobkaufmann August 8, 2023 00:18

carver added a commit to carver/utp that referenced this pull request Aug 8, 2023

Close without error if just missing FIN-ACK

8929d8b

Close errors will be used more, when ethereum#98 lands, which returns the close error to clients who wait for the stream to exit.

Prefer close() to shutdown(), it waits for finish

778120f

stream.close().await will not return until the connection exits, either from an error, or from all data being written and ACKed.

carver force-pushed the blocking-close-api branch from 14ddfd0 to 0d2a6da Compare August 8, 2023 17:10

jacobkaufmann reviewed Aug 8, 2023

View reviewed changes

carver force-pushed the blocking-close-api branch from 0d2a6da to 7a3fb10 Compare August 9, 2023 01:38

carver merged commit 750bd14 into ethereum:master Aug 9, 2023

carver deleted the blocking-close-api branch August 9, 2023 01:41

carver added a commit to carver/utp that referenced this pull request Aug 9, 2023

Close without error if just missing FIN-ACK

5409e04

Close errors will be used more, when ethereum#98 lands, which returns the close error to clients who wait for the stream to exit.

carver added a commit to carver/utp that referenced this pull request Aug 9, 2023

Close without error if just missing FIN-ACK

4780736

Close errors will be used more, when ethereum#98 lands, which returns the close error to clients who wait for the stream to exit.

carver added a commit to carver/utp that referenced this pull request Aug 9, 2023

Close without error if just missing FIN-ACK

e92240a

Close errors will be used more, when ethereum#98 lands, which returns the close error to clients who wait for the stream to exit.

carver added a commit to carver/utp that referenced this pull request Aug 9, 2023

Close without error if just missing FIN-ACK

10b7cff

Close errors will be used more, when ethereum#98 lands, which returns the close error to clients who wait for the stream to exit.

carver added a commit to carver/utp that referenced this pull request Aug 9, 2023

Close without error if just missing FIN-ACK

b6d9fef

Close errors will be used more, when ethereum#98 lands, which returns the close error to clients who wait for the stream to exit.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prefer close() to shutdown(), it waits for finish #98

Prefer close() to shutdown(), it waits for finish #98

carver commented Jul 27, 2023

jacobkaufmann left a comment

jacobkaufmann Aug 3, 2023

jacobkaufmann left a comment

jacobkaufmann Aug 5, 2023

jacobkaufmann Aug 5, 2023

carver Aug 7, 2023

jacobkaufmann Aug 8, 2023

jacobkaufmann Aug 6, 2023

jacobkaufmann Aug 6, 2023

jacobkaufmann Aug 6, 2023

jacobkaufmann Aug 6, 2023

carver Aug 7, 2023

jacobkaufmann Aug 6, 2023

carver Aug 7, 2023

jacobkaufmann Aug 8, 2023

jacobkaufmann Aug 6, 2023

carver Aug 7, 2023 •

edited

Loading

jacobkaufmann Aug 8, 2023

carver commented Aug 8, 2023 •

edited

Loading

carver commented Aug 8, 2023

jacobkaufmann left a comment

jacobkaufmann Aug 8, 2023

jacobkaufmann Aug 8, 2023

jacobkaufmann Aug 8, 2023

jacobkaufmann Aug 8, 2023

jacobkaufmann Aug 8, 2023

carver commented Aug 9, 2023

	async fn that_close_errors_if_all_packets_dropped() {
	async fn close_errors_if_all_packets_dropped() {

	/// Close this connection, after finishing the pending writes
	/// Closes the stream gracefully.
	/// Completes when the remote peer acknowledges all sent data.

		let result = self.outbound.send(buf.to_vec());
		if let Err(err) = result {

	let result = self.outbound.send(buf.to_vec());
	if let Err(err) = result {
	if let Err(err) = self.outbound.send(buf.to_vec()) {

		// After adding some mechanism that sends a RESET packet, add the following test:
		// Test that close() returns ConnectionReset if recipient resets during transfer

		pub fn build_link_pair() -> (MockUdpSocket, MockUdpSocket) {
		let (peer_a, peer_b): (char, char) = ('A', 'B');

	pub fn build_link_pair() -> (MockUdpSocket, MockUdpSocket) {
	let (peer_a, peer_b): (char, char) = ('A', 'B');
	pub fn mock_link((a, b): (char, char)) -> (MockUdpSocket, MockUdpSocket) {

	pub fn get_status_controller(&mut self) -> Arc<AtomicBool> {
	pub fn up_status(&mut self) -> Arc<AtomicBool> {

Prefer close() to shutdown(), it waits for finish #98

Prefer close() to shutdown(), it waits for finish #98

Conversation

carver commented Jul 27, 2023

jacobkaufmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobkaufmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carver Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carver commented Aug 8, 2023 • edited Loading

carver commented Aug 8, 2023

jacobkaufmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carver commented Aug 9, 2023

carver Aug 7, 2023 •

edited

Loading

carver commented Aug 8, 2023 •

edited

Loading