Fix infinite loop on fast TCP disconnection #557
Merged
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
The commit a841b28 changed the condition for removing job from processing. New flag MultiplexerJobStatus::continue_servicing become used instead of checking pointer for NULL. However for cases when TCPSocket::newJob() returns nullptr the behaviour changed: earlier the job was removed, but after change it is called again, since MultiplexerJobStatus equal to {true, nullptr} means "run this job again". This leads to problem with eating CPU and RAM on linux debauchee#470 There is similar windows problem, but not sure it is related. debauchee#552 Since it looks that the goal of a841b28 was only clarifying object ownership and not changing job deletion behaviour, this commit tries to get original behaviour and fix the bugs above by returning {false, nullptr} instead of {true, nullptr} when TCPSocket::newJob() returns nullptr.
|
This should fix #470. However the issue was rarely-reproducible for me, so some more testing may be needed. Does changing the logic in a841b28 when newJob() returns nullptr (treated as no new job) to {true, nullptr} (treated as retry current job ) was unintentional side effect or was explicit goal to fix some behaviour? |
|
@galkinvv: This is really good analysis. I think this issue was indeed my oversight. I would have made a separate commit for any change in semantics. |
p12tic
approved these changes
Feb 13, 2020
|
@AdrianKoshka: I think you can merge this one. |
This was referenced May 6, 2020
This was referenced Nov 10, 2021
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.
The commit a841b28 changed the condition for removing job from processing.
New flag MultiplexerJobStatus::continue_servicing become used
instead of checking pointer for NULL.
However for cases when TCPSocket::newJob() returns nullptr
the behaviour changed: earlier the job was removed, but after change
it is called again, since MultiplexerJobStatus equal to {true, nullptr}
means "run this job again".
This leads to problem with eating CPU and RAM on linux
#470
There is similar windows problem, but not sure it is related.
#552
Since it looks that the goal of a841b28 was only clarifying
object ownership and not changing job deletion behaviour,
this commit tries to get original behaviour and fix the bugs above
by returning {false, nullptr} instead of {true, nullptr}
when TCPSocket::newJob() returns nullptr.