Close the links after closing the Face #1979
Open
Conversation
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
This is an attempt to minimize the occurrence of the race condition described in eclipse-zenoh#1886.
|
PR missing one of the required labels: {'dependencies', 'breaking-change', 'enhancement', 'internal', 'bug', 'new feature', 'documentation'} |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1979 +/- ##
==========================================
- Coverage 71.02% 70.89% -0.14%
==========================================
Files 364 364
Lines 61519 61519
==========================================
- Hits 43693 43612 -81
- Misses 17826 17907 +81 ☔ View full report in Codecov by Sentry. |
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.
This is another attempt to minimize the occurrence of the race condition described in #1886.
When loosing the connection to a peer, if the send buffer happens to fill up before the keep alive timeout, the tx_task can be stuck in the send_batch operation. On my computer, it can be stuck for 15 minutes before returning with EHOSTUNREACH ("No route to host").
Now this is not that bat itself, it's just a dangling task, a TransportLinkUnicastUniversal and a TransportUnicastUniversal. But this make the race condition mentioned in #1886 very likely: if the peer reconnect within those 15 minutes, the re-connection is bogus.
This "fix" make
TransportUnicastUniversal::delete()notify the RuntimeSession of the transport deletion before closing the link, instead of after closing the link. It doesn't seem to cause any issue. I didn't do the same in TransportUnicastLowlatency because I am not sure it is necessary.I am not really happy with this hack, because I don't fully understand the consequences of the fix and it just make the race-condition a lot less likely (15min time span vs a few milliseconds). Have a look at #1886 for more information.