How to distinguish FIN and RST closes in recv?

[Dirbaio]

When reading data from a TcpSocket with the recv_* functions, Error::Illegal is returned when there's no more readable data.

The issue is AFAICT there's no way to distinguish whether the remote end gracefully closed the connection with a FIN packet, or the connection was aborted with a RST packet, or other error cases (idle timeout, etc).

A common use case for this is when downloading a file. You want to know whether you have received the whole file (gracefully closed with FIN) or only part of it because the connection died for some other reason, so you don't silently truncate the file.

• Is it correct that this is currently impossible?
• How would the API for this look? Something like recv_* functions being able to signal EOF vs an error condition, like the Berkeley sockets API.

[whitequark]

If I recall correctly, once you've exhausted the receive buffer, if the remote end sent FIN, then sock.is_open() && !sock.may_recv(); if the remote end sent RST, then !sock.is_open().

[Dirbaio]

That won't work if we've closed our side before. Consider this:

• open a connection to a server. -> state ESTABLISHED.
• write some data corresponding to a "request" of whatever protocol we're implementing.
• call close() to signal we're done writing the request. -> send FIN, state FIN_WAIT_1.
• server ACKs our FIN -> state FIN_WAIT_2.
• server sends response data
• server sends FIN to signal it's done sending response data -> state TIME_WAIT.
• application calls sock.recv(), gets some data. rx_buffer is now empty.
• application calls sock.recv(), gets Error::Illegal
• application wants to know if it's got all the data, but:
  • sock.is_open() = false (because we're in TIME_WAIT)
  • sock.may_recv() = false

... which can happen both with receiving FIN or RST.

[whitequark]

Yep, that's true. Let's figure out an API for this case, then.

[Dirbaio]

Option 1: Signal EOF with a new error

• Add Error::EOF
• recv_*() returns:
  • N bytes if rx_buffer has data.
  • 0 bytes if rx_buffer is empty but receive half is not closed yet.
  • Error::EOF if rx_buffer is empty and receive half is gracefully closed by FIN. (breaking change)
  • Error::Illegal if rx_buffer is empty and receive half is non-gracefully closed (RST or timeout).

Option 2: Signal EOF by returning 0 bytes, like std::io::Read.

• Add Error::WouldBlock
• recv_*() returns:
  • N bytes if rx_buffer has data.
  • Error::WouldBlock if rx_buffer is empty but receive half is not closed yet. (breaking change)
  • 0 bytes if rx_buffer is empty and receive half is gracefully closed by FIN. (breaking change)
  • Error::Illegal if rx_buffer is empty and receive half is non-gracefully closed (RST or timeout).

Option 1 is closest to the current smoltcp behavior.
Option 2 is closest to the std io traits, which might make it more uniform with the ecosystem.

[whitequark]

Both of these are changes that would break all existing code, right?

[Dirbaio]

Yes :(

Option 1 would be the least breaking. It won't break code that handles any recv error as "okay, no more data" but it will break code that looks at what actual error is returned.

There's Option 3:

• Leave recv_*() methods as is
• add a is_eof() method that returns true if rx_buffer is empty and receive half is gracefully closed by FIN.

Hoever, it feels weird to me to have the extra is_eof() method. It's very unlike any other io api/trait out there. All of them signal EOF in the return value of read calls.

What's the project's policy on breaking changes? My PR #346 is a breaking change too.

[whitequark]

What's the project's policy on breaking changes? My PR #346 is a breaking change too.

Breaking changes on the system/PHY side are generally fine as long as they aren't silently breaking code. Breaking changes on the socket side are much less fine, especially if they do silently break existing code.

Hoever, it feels weird to me to have the extra is_eof() method. It's very unlike any other io api/trait out there. All of them signal EOF in the return value of read calls.

I've considered these options and I think I would prefer adding Error::Finished for this case, alongside Error::Illegal. Error::WouldBlock doesn't fit the smoltcp model at all, and the is_eof method, while is not too radical of an addition (we already have a bunch of those), seems like it would make using the library harder than necessary.

[Dirbaio]

Breaking changes on the system/PHY side are generally fine as long as they aren't silently breaking code. Breaking changes on the socket side are much less fine, especially if they do silently break existing code.

Makes sense, thanks for the clarification.

I would prefer adding Error::Finished for this case, alongside Error::Illegal

Cool! I'll try to send a PR.

There are hairy edge-cases, like: we receive 3 packets: seq 0-100, seq 200-300 with FIN, and a RST. In this case the app should read 0-100 and then get Err::Illegal even if we've received a FIN, because it hasn't read the entire data because we're missing a fragment.

[whitequark]

Yep, this isn't very easy to implement unfortunately.