Skip to content

Conversation

@willmmiles
Copy link

Defer closing the connection to the async thread when processing a fin event. This allows client connections to process all received data before the connection is terminated.

This can be slightly more expensive for servers who are handling unexpected client termination as they may still process now-pointless ack events, but should have no effect on the normal case (as the server closes when it's done sending). I did a quick perftest with AsyncWebServer and saw no significant performance difference.

@me-no-dev
Copy link
Member

Only concern is to not try to access the now probably gone pcb

@willmmiles
Copy link
Author

Only concern is to not try to access the now probably gone pcb

The FIN event doesn't invalidate the pcb; a half-closed connection is a legal TCP state. LwIP handles this correctly. The pcb won't be invalidated until we call tcp_close() or tcp_abort() on it. It's only the tcp_err() callback that forces us to deal with a pcb that's going to be invalidated by the library without us explicitly calling in to release it.

Historically, though, half-closed connections have been a somewhat curious corner of the TCP standard. This is because the traditional UNIX sockets API can't represent it: FIN is sent when close() is called on a socket, after which it can no longer read() any data sent by the other side (because the file handle is invalidated). So it's not entirely unreasonable for the remote partner to assume that FIN means "nothing I send now will be visible to the client", therefore I might as well close up this connection. But that is a sockets-ism; it's perfectly legal in the TCP standard for a connection to continue to process and ACK incoming data after sending a FIN to its partner to let it know that its side is finished sending data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants