Skip to content
This repository has been archived by the owner on Jan 6, 2020. It is now read-only.

Commit

Permalink
Revert "Return error if connection got closed while receiving"
Browse files Browse the repository at this point in the history 
This reverts commit 7e44f93.
  • Loading branch information
@vinipsmaker
vinipsmaker committed Nov 20, 2015
1 parent 5b0b85a commit f64d250
Showing 1 changed file with 2 additions and 3 deletions.
5 changes: 2 additions & 3 deletions src/socket.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -484,10 +484,9 @@ impl UtpSocket {
}

loop {
// We either received a confirmation for a Fin packet we
// sent or max_retransmission_retries has been reached.
// A closed socket with no pending data can only "read" 0 new bytes.
if self.state == SocketState::Closed {
return Err(Error::from(SocketError::ConnectionClosed));
return Ok((0, self.connected_to));
}

let user_read_timeout = self.user_read_timeout;
Expand Down

7 comments on commit f64d250

@inetic
Copy link

@inetic inetic commented on f64d250 Dec 22, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should discuss this. Why was it reverted? Was it causing bugs somewhere else? If so, I believe it should have been fixed there, because:

  1. Treating Ok(0, _) as an error goes against the Error semantics.
  2. Treating Ok(0, _) as an error is not a standard for any read operation
  3. Treating Ok(0, _) as an error makes it impossible for the remote peer to send us a packet with zero size payload.

@vinipsmaker
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was it reverted?

It broke most of the unit testing code within rust-utp. Mostly this function: https://doc.rust-lang.org/stable/std/io/trait.Read.html#method.read_to_end

  1. Treating Ok(0, _) as an error goes against the Error semantics.

Ok(0_) is being treated as EOF, not as an error. Just like the Rust documentation tell us so.

  1. Treating Ok(0, _) as an error is not a standard for any read operation

It is not being treated as an error. Take a look at this function: https://doc.rust-lang.org/stable/std/io/trait.Read.html#method.read_to_end

  1. Treating Ok(0, _) as an error makes it impossible for the remote peer to send us a packet with zero size payload.
  1. But it is not being treated as an error.
  2. That's why it's a blocking call. It'll block until some byte is received. That's also why blocking calls are problematic. It doesn't let us easily to break out of a blocking operation.
  3. The abstraction is designed in such a way that knowing that a zero size payload would only be useful to detect activity and timeout, but such behaviour (connection lost/timed-out). And detecting connection lost/timed-out ourselves would be a too complex task, as it'd force us to implement the congestion control logic from uTP in the crust side, which completely breaks the separation of responsibilities.

@inetic
Copy link

@inetic inetic commented on f64d250 Dec 23, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Treating Ok(0, _) as an error goes against the Error semantics.

Ok(0_) is being treated as EOF, not as an error. Just like the Rust documentation tell us so.

But uTP sockets are datagram based, not stream based, I don't believe EOF applies here (maybe I'm missing something?).

Also, even if we treated it as a stream (which we really shouldn't), as you noted, there is a difference between EOF and "file/stream closed". The latter should be treated as error (AFAIK, it always is in every API I've worked with). And this line suggests that we tried to read, but the socket was already closed.

That's why it's a blocking call. It'll block until some byte is received. That's also why blocking calls are problematic. It doesn't let us easily to break out of a blocking operation.

That is true, but I don't see why it is relevant here. If we try to read from the socket and we don't receive anything within a user defined timeout then that should result in a TimedOut error.

The abstraction is designed in such a way that knowing that a zero size payload would only be useful to detect activity and timeout, but such behaviour (connection lost/timed-out).

Not sure I understand, yet again, if these sockets were stream based, then I would say you're right, but they are datagram based and in datagram world, sending zero sized payload is perfectly valid.

And detecting connection lost/timed-out ourselves would be a too complex task, as it'd force us to implement the congestion control logic from uTP in the crust side, which completely breaks the separation of responsibilities.

Here I probably also don't understand. What is it that would force us to implement congestion control logic on crust side?

@vinipsmaker
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But uTP sockets are datagram based, not stream based, I don't believe EOF applies here (maybe I'm missing something?).

Wrong. UDP sockets are datagram based. uTP sockets add congestion control and reliability (including delivery order). EOF are used in the tests. If you use your commit and run rust-utp tests, lots of them will fail.

Also, even if we treated it as a stream (which we really shouldn't), as you noted, there is a difference between EOF and "file/stream closed". The latter should be treated as error (AFAIK, it always is in every API I've worked with).

From the text:

Read all bytes until EOF [...] This function will continuously call read to append more data to buf until read returns either Ok(0) or an error of non-ErrorKind::Interrupted kind.

We can know that Rust treats Ok(0) as EOF.

I believe the point of EOF in Rust is that, unless you do something else (like seek(0) in a File-based implementation), you'll never get bytes again. That's why read_to_end will work under such behaviour of Ok(0, _).

About "it always is in every API I've worked with", I'm on "your boat too". That's why I didn't put the Ok(0, _) in crust since the beginning. But if it's Rust convention to do in such way, it's what we should follow.

And this line suggests that we tried to read, but the socket was already closed.

If you see the code, you'll see that control packets are never returned and the function blocks until some byte is read, EOF is reached or an error happens. An error will be returned if UdpSocet::recv_from returns an error different thant ErrorKind::WouldBlock | ErrorKind::TimedOut.

If we try to read from the socket and we don't receive anything within a user defined timeout then that should result in a TimedOut error.

Upstream rust-utp doesn't have timeouts. Our implementation does have timeouts and we do return an Err if timeout is reached with no successful read operation.

Not sure I understand, yet again, if these sockets were stream based, then I would say you're right, but they are datagram based and in datagram world, sending zero sized payload is perfectly valid.

We're understanding each other.

From the text:

The main difference compared to TCP is the delay based congestion control.

If it was "the only difference", I could prove my point that uTP (just like TCP) is stream-based. However, if you read the rest of the text, you'll see that uTP don't try to be datagram-based at all. The underlying implementation is datagram-based and a low-level API could expose such behaviour, but rust-utp is higher-level. There's even a test to ensure delivery order. If that's not enough, you can get a UtpStream from a UtpSocket at almost no changes (the same behaviour is preserved).

Here I probably also don't understand. What is it that would force us to implement congestion control logic on crust side?

Let's leave this comment for later or never. I was trying to anticipating your comments/intention. Let's first agree on uTP being datagram/stream based and maybe we can return to this point later.

@inetic
Copy link

@inetic inetic commented on f64d250 Dec 24, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just did a small test, to decide whether UtpSocket is really a stream or message passer. In the test the sender sent one message of size 20 bytes and the receiver tried to receive twice into a buffer of size 10 bytes. If the socket was message based (I'm switching the terminology from datagram to message as datagram is often associated with non reliability), then I would only receive once and the second call would block. On the other hand, if it was a stream, I would be able to receive twice.

To my surprise, it was indeed behaving like a stream. I personally think this is a bad design decision from the library creator, but oh well...

uTP sockets add congestion control and reliability (including delivery order)

I think it is important to note that reliability and congestion control are not mutually exclusive with being message based. Or put differently: reliability and congestion control do not imply stream based. On the contrary, I had expected that UtpSocket was reliable message passer and then UtpStream was stream based (note that the terminology would correspond to UdpSocket and TcpStream Rust objects). This would make a lot of sense to me because it is efficient to implement stream on top of a reliable message passer, but to do it the other way around you'd need to add bytes indicating where a message ends (so less efficient). Also, having a separate UtpStream object where UtpSocket is also a stream seems kind of pointless, but again, oh well... :)

We can know that Rust treats Ok(0) as EOF.

I am not claiming otherwise here, what I am saying is that what happens in this commit doesn't look like EOF. I'll elaborate...

I believe the point of EOF in Rust is that, unless you do something else (like seek(0) in a File-based implementation), you'll never get bytes again.

That is not the complete story. If you're reading from a file, and you hit EOF, you can still read more data out of it if another thread or process appends to the file. See this example. Note that this is not what the lines in this commit are about. That is, if the socket is closed and we try to receive, we'll get EOF (Ok(0, _)), but we'll never receive any more data out of it, even if the other end tries to send more.

Now imagine we have one file handle shared by two threads, one thread writes to it and the other end reads from it. Then, when the writing thread is done, it closes the file handle. After that, (IMO) the reading thread should get an error, not EOF, because the reader wont be able to read any more bytes out of the handle. This other scenario is - I believe - more alike to what is happening in this commit.

If I'm right, and we revert the commit instead of modifying the tests, we'll be creating a technical debt. It probably wouldn't be too big of a deal, as we're already in debt by using blocking uTP API (it's still an itch though).

@vinipsmaker
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is important to note that reliability and congestion control are not mutually exclusive [...]

True.

[...] If I'm right, and we revert the commit instead of modifying the tests, we'll be creating a technical debt.

I agree that your arguments on API design are very appealing. Unfortunately, rust-utp follows TcpStream design closer than your suggestion. My little test shows that.

Fortunately (somehow), the blocking call ensures Ok(0) will only be returned when EOF/socket-closed is reached. It'll loop forever until such condition happens. That's the reason why this revert works well.

@inetic
Copy link

@inetic inetic commented on f64d250 Jan 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that your arguments on API design are very appealing. Unfortunately, rust-utp follows TcpStream design closer than your suggestion. My little test shows that.

This is interesting, I was tracing where in the rust code this happens. On Linux, this is done implicitly here:

https://github.com/rust-lang/rust/blob/master/src/libstd/sys/unix/net.rs#L110

And it seems to be the default posix behavior

If all file descriptors referring to the write end of a pipe have been closed, then an attempt to read(2) from the pipe will see end-of-file (read(2) will return 0).

The behavior I was describing seems to be default for windows as this code suggests. I hate to say it, but I like the Windows one better :). But yeah, as you say, we should stick with what rust uses as standard.

Please sign in to comment.