fix: fail fast when pushing to a transport being closed as unresponsive - #2709
Open
Anishishi wants to merge 1 commit into
Open
fix: fail fast when pushing to a transport being closed as unresponsive#2709Anishishi wants to merge 1 commit into
Anishishi wants to merge 1 commit into
Conversation
When a non droppable message cannot be pushed within wait_before_close, the transport is closed. However, the transmission pipelines remained enabled until the close task actually ran, so every subsequent push to the unresponsive transport blocked for another full wait_before_close period. This is especially harmful when the pushing threads are RX runtime workers holding routing locks (e.g. forwarding reliable data or declarations under the routing tables lock): they serially block on the dead transport and starve the RX runtime, preventing the spawned close task from ever running and deadlocking the whole session (eclipse-zenoh#1876, eclipse-zenoh#2581). Synchronously mark the transmission pipelines as disabled when the closure is initiated, so that subsequent pushes fail fast with TransportClosed and the close task can proceed. Signed-off-by: Anishishi <aki.nishi.work@gmail.com>
Author
|
Could someone with triage/write access please add the I don't have permission to add labels myself, and the checklist CI (
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When a peer's TCP connection stays established but the peer stops reading (zero-window; e.g. a frozen or abruptly power-cycled host), pushes of non droppable messages to that transport block for
wait_before_close. When the deadline expires,TransportUnicastUniversal::handle_push_resultlogsUnable to push non droppable network message ... Closing transport!and spawns the close task on the RX runtime.However, the transmission pipelines of the transport remain enabled until the close task actually runs. If the threads pushing to the dead transport are RX runtime workers (forwarding reliable data or declarations, holding routing locks), they immediately block again on the next message for another full
wait_before_closeeach, starving the RX runtime. The close task never runs and the whole session is livelocked, logging the message above forever.Fixes #1876, related to #2581.
Fix
Synchronously mark the transmission pipelines of the transport as disabled at the moment the closure is decided. This is a new
TransmissionPipelineProducer::mark_disabledmethod that only performs an atomic store; contrary todisableit does not acquire thestage_inlocks (which may be held by a currently blocked pusher).push_network_messagenow fails fast withTransportClosedwhen the pipeline is disabled, before attempting to acquire thestage_inlock. Subsequent pushes to the unresponsive transport therefore return immediately, the RX workers are freed, and the spawned close task can run and perform the regular cleanup.No behavior change for healthy transports: the check is a relaxed atomic load on the already-existing status flag.
Reproducer
A standalone reproducer with stock
zenohd+ examples (SIGSTOP-ed subscriber, blocking publishers, liveness probe) is attached to #1876. On currentmainthe router wedges forever, alternating the error log between the RX workers everywait_before_close; with this PR it recovers right afterwait_before_close.Tests
unicast_unresponsive_peersimulates the zero-window peer with a stallable TCP proxy and asserts that, after the push failure that triggers the closure, the next push fails fast instead of blocking for anotherwait_before_close. It fails on currentmainand passes with this PR.cargo test -p zenoh-transport --features transport_tcppasses.rmw_zenohdeployment: 100 abrupt reboot cycles with zero occurrences of the wedge (previously reproduced within a few dozen cycles).🏷️ Label-Based Checklist
Based on the labels applied to this PR, please complete these additional requirements:
Labels:
bug🐛 Bug Fix Requirements
Since this PR is labeled as a bug fix, please ensure:
Why this matters: Bugs without tests often reoccur.
Instructions:
- [ ]to- [x])This checklist updates automatically when labels change, but preserves your checked boxes.