enhancement: Detect TCP disconnects earlier in socket and vector sinks #2209
Conversation
This file contains 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
added 2 commits
April 2, 2020 14:22
Signed-off-by: Bruce Guenter <bruce@timber.io>
Signed-off-by: Bruce Guenter <bruce@timber.io>
bruceg
added
sink: socket
|
Would it be helpful/eaier to test this in the test harness? It's definitely more of a pain to do, but makes full integration tests like this possible. |
|
I don't see how the test harness would make any difference. Without explicit acknowledgements in the protocol, there will always be a race between the remote closing the connection (and so dropping anything else sent on the socket) and this sink detecting the closure (and so dropping anything already sent). The purpose of this PR is to narrow the gap, but it cannot be closed. The question I was raising was how to reliably determine that the gap has been narrowed. |
lukesteensen
approved these changes
Apr 3, 2020
|
Nice work! Glad we got this resolved. |
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.
Closes #1429
It is unclear to me how this could be reliably tested. I could probably craft up a receiver that would drop the connection mid stream, but this would produce all manner of races. It would also be difficult (impossible?) to tell the difference between the normal disconnect handling and this enhancement.