Skip to content

tcp: connections are never abandoned when the sender retries a blocked send #410

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
tinic wants to merge 1 commit into from
Closed

tcp: connections are never abandoned when the sender retries a blocked send #410

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions common/src/nx_tcp_socket_retransmit.c
View file
Open in desktop

@fdesbiens fdesbiens Aug 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You offer to contribute the test in whatever form suits the project, and I would like to take that up rather than merge without it. The fix is two guards whose necessity is invisible from the surrounding code. Someone will eventually read if ((tx_window_advertised == 0) && (has_data == NX_FALSE)) and decide the first clause is redundant. Only a test stops that.

Your three cases are already the right set, and the third is the one I would insist on — a fix that abandons a peer legitimately holding a zero window would be worse than the bug, and that case is what proves it does not.

On where it goes: test/regression/netxduo_test/ is the home, driven by _nx_ram_network_driver over two NX_IP instances, which is how the TCP tests in that directory work. Two things should make the port easier than it sounds:

  • There is precedent for reaching into socket state directly. netx_tcp_branch_test.c:1003 sets nx_tcp_socket_zero_window_probe_failure to nx_tcp_socket_timeout_max_retries by hand to reach a branch, so a test that inspects or nudges these fields is in keeping with the suite.
  • Your approach of stepping the timer rather than waiting for it is also already used — several tests in that directory manipulate timing rather than sleeping through it — so 600 simulated seconds in a fraction of a second should be achievable without your downstream shim.

If the shim approach does not translate, an acceptable narrower version would be a white-box test that drives _nx_tcp_fast_periodic_processing() directly with the socket in each of the three states and asserts whether _nx_tcp_socket_connection_reset() was reached. Less faithful than yours, but it would pin the guards.

@fdesbiens fdesbiens Aug 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Applies to common/src/nx_tcp_socket_retransmit.c:133-134.

I went looking for a way your fix could make something else worse, found this, and worked out that it cannot. Recording it so the analysis is not repeated.

During a genuine persist against a responsive peer, nx_tcp_socket_timeout_retries is incremented at retransmit.c:129 on every probe and is not reset by the ACK path — state_ack_check.c:588-601 only clears it when the ACK releases queued packets, which a pure probe ACK does not do. It then feeds the shift at :133:

socket_ptr -> nx_tcp_socket_timeout = socket_ptr -> nx_tcp_socket_timeout_rate <<
    (socket_ptr -> nx_tcp_socket_timeout_retries * socket_ptr -> nx_tcp_socket_timeout_shift);

With timeout_shift of 1 that is a shift of 32 once timeout_retries reaches 32, which is undefined behaviour on a 32-bit ULONG.

It is not reachable, though. Because each interval is twice the last, getting to the 32nd probe takes on the order of 2^32 ticks — over a century at a one-second rate. And with timeout_shift of 0 the shift count is always 0, so that configuration is safe by construction.

Importantly it is also unchanged by your patch: before it, a legitimate persist survived indefinitely too, because probe_failure was pinned at 1 and has_data was TRUE. So this is strictly pre-existing and your fix neither introduces nor extends it. No action needed; I mention it only because it lives in the lines you are editing and someone will eventually ask.

Original file line number Diff line number Diff line change
Expand Up @@ -110,9 +110,21 @@ ULONG window_size;
socket_ptr -> nx_tcp_socket_zero_window_probe_data = *(packet_ptr -> nx_packet_prepend_ptr + ((header_ptr -> nx_tcp_header_word_3 >> 28) << 2));

/* Now set zero window probe started. */
socket_ptr -> nx_tcp_socket_zero_window_probe_has_data = NX_TRUE;
socket_ptr -> nx_tcp_socket_zero_window_probe_sequence = header_ptr -> nx_tcp_sequence_number;
socket_ptr -> nx_tcp_socket_zero_window_probe_failure = 0;

/* The failure count belongs to the probe, not to each attempt at
it: clear it only when a new probe starts, as the two places in
nx_tcp_socket_send_internal.c that arm one do. Clearing it
here on every attempt pinned it at one, so the retry limit that
_nx_tcp_fast_periodic_processing() tests against it during a
zero window could never be reached and a peer that stopped
answering its probes was never given up on. A peer that does
answer still clears it (nx_tcp_socket_state_ack_check.c). */
if (socket_ptr -> nx_tcp_socket_zero_window_probe_has_data == NX_FALSE)
{
socket_ptr -> nx_tcp_socket_zero_window_probe_has_data = NX_TRUE;
socket_ptr -> nx_tcp_socket_zero_window_probe_failure = 0;
}

NX_CHANGE_ULONG_ENDIAN(header_ptr -> nx_tcp_sequence_number);
NX_CHANGE_ULONG_ENDIAN(header_ptr -> nx_tcp_header_word_3);
Expand Down
20 changes: 17 additions & 3 deletions common/src/nx_tcp_socket_send_internal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1017,7 +1017,16 @@ UINT compute_checksum = 1;
/* Increment the suspended thread count. */
socket_ptr -> nx_tcp_socket_transmit_suspended_count++;

if (socket_ptr -> nx_tcp_socket_zero_window_probe_has_data == NX_FALSE)
/* Only a receiver that advertised a zero window is probed. This
data cannot be sent for one of three reasons -- the receiver's
window, the congestion window, or the transmit queue depth --
and only the first of them is a zero window. Setting the flag
for the other two describes the socket as being in the persist
state when it is not, which moves the retransmission retry
limit onto the probe failure count (see
nx_tcp_fast_periodic_processing.c) and stops it being reached. */
if ((socket_ptr -> nx_tcp_socket_tx_window_advertised == 0) &&

@fdesbiens fdesbiens Aug 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have raised this on two of your other PRs, so treat it as a consistent preference rather than a new objection — and here I think it is a closer call than usual, so I am not going to press it.

The three added comments run to eight lines each and most of the text explains what used to go wrong and which counter it moved the limit onto. That belongs in the commit message, where you have already written it better than the comment does.

What earns its place in the source is the invariant: a probe belongs to a zero window and to nothing else, and the flag must not be set for congestion or queue depth. Two lines. The reason I am softer on this one than on #401 is that the invariant genuinely is non-obvious and the comment is doing real work in protecting it — I would rather have a slightly overlong comment here than none. Trim the history, keep the rule.

One small thing while you are editing them: nx_tcp_fast_periodic_processing.c is referenced in prose in both files, which is helpful, but the reference in retransmit.c:123 points at nx_tcp_socket_state_ack_check.c without a line and the one in send_internal.c:1028 names no line either. Since you have the line numbers to hand, they would save the next reader a grep.

(socket_ptr -> nx_tcp_socket_zero_window_probe_has_data == NX_FALSE))
{

/* Set data for zero window probe. */
Expand Down Expand Up @@ -1061,8 +1070,13 @@ UINT compute_checksum = 1;
else
{

/* Check advertised window. */
if (socket_ptr -> nx_tcp_socket_zero_window_probe_has_data == NX_FALSE)
/* Check advertised window. As above, a probe belongs to a zero
window and to nothing else: the caller is about to be told
NX_WINDOW_OVERFLOW or NX_TX_QUEUE_DEPTH and will try again, and
each of those attempts would otherwise re-declare a persist
state that hides the retransmission retry limit. */
if ((socket_ptr -> nx_tcp_socket_tx_window_advertised == 0) &&
(socket_ptr -> nx_tcp_socket_zero_window_probe_has_data == NX_FALSE))
{

/* Set data for zero window probe. */
Expand Down