Kernel/Net: Implement SO_LINGER support in TCPSocket close behavior #25037
Conversation
logkos
force-pushed
the
lingmyding
branch
2 times, most recently
from
f05d6e0
to
e365670
Compare
github-actions
bot
added
the
👀 pr-needs-review
|
Hello! One or more of the commit messages in this PR do not match the SerenityOS code submission policy, please check the |
nico
added
⏳ pr-waiting-for-author
|
(looks like the updated commit description has lines longer than 72 chars, making CI unhappy.) |
github-actions
bot
added
👀 pr-needs-review
|
Looks like you also need to use "Kernel/Net: " instead of "Net: " as prefix in the commit title. |
nico
added
⏳ pr-waiting-for-author
logkos
changed the title
github-actions
bot
added
👀 pr-needs-review
logkos
force-pushed
the
lingmyding
branch
5 times, most recently
from
e9749c7
to
6cb4b8c
Compare
|
Nice work! This was one of the (many) blockers needed to make my asynchronous sockets work on Serenity. I would love to see a test for this though, something like: create a TCP socket pair, make one of the ends non-blocking with {0,x} second lingering, sends a ton of data to overflow receive/send buffers, close that end, poll the other end and see how fast POLLERR arrives. |
DanShaders
added
⏳ pr-waiting-for-author
github-actions
bot
added
👀 pr-needs-review
| } | ||
| } else { | ||
| dbgln_if(TCP_SOCKET_DEBUG, "SO_LINGER without timeout, closing immediately"); | ||
| (void)send_tcp_packet(TCPFlags::RST); |
There was a problem hiding this comment.
where are these reset packets coming from, that doesn’t look very state-machine correct? see the code below that follows TCP state machine behavior.
| (void)Thread::current()->sleep(Duration::from_milliseconds(1)); | ||
| } while (TimeManagement::the().monotonic_time() < deadline); | ||
|
|
||
| if (has_unacked_data) { |
There was a problem hiding this comment.
you never re-check the unacked data, meaning this variable will likely not correctly reflect the current state of things at this point (likely, most data was acked by now, but that’s not checked).
Kernel/Net/TCPSocket.cpp
Outdated
| if (!has_unacked_data) | ||
| break; |
There was a problem hiding this comment.
this check seems pointless and can just be part of the while instead
|
|
||
| if (has_unacked_data) { | ||
| dbgln_if(TCP_SOCKET_DEBUG, "SO_LINGER timeout. Some data couldn't do it :("); | ||
| (void)send_tcp_packet(TCPFlags::RST); |
There was a problem hiding this comment.
I’m also not sure whether this is state-machine-correct behavior? are we not supposed to send just a FIN in this case?
SO_LINGER allows specifying a timeout interval during which the socket attempts to send any remaining data before closing. If all data is acknowledged within the specified timeout, the connection is closed gracefully using a FIN/ACK exchange. If the timeout expires with unacknowledged data or l_linger set to 0, the socket is forcefully closed using RST.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions! |
github-actions
bot
removed
the
👀 pr-needs-review
This PR introduces a simple option that changes the behavior of
close()for connection-oriented protocols, allowing us to specify a timeout interval for packets to be acknowledged or let us forcefully close the socket with a RST.This feature is particularly useful for the Dropbear and libuv ports and enables more advanced socket operations. I specifically need this for porting a mainframe emulator.