network: reintroduce #4382 (delayed conn close) with segv fix#4587
Conversation
Re-enable the changes reverted in 9d32e5c, which were originally merged as part of envoyproxy#4382. Signed-off-by: Andres Guedez <aguedez@google.com>
Fixes a segfault introduced in envoyproxy#4382 due to a connection tear down race condition when the delayed close timer triggers after connection state has been reset via closeSocket(). Signed-off-by: Andres Guedez <aguedez@google.com>
|
asan/tsan failing due to an issue with one of the tests I added; I'm working on a fix. |
Signed-off-by: Andres Guedez <aguedez@google.com>
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thanks for fixing the bug!
| // It's ok to disable even if the timer has already fired. | ||
| delayed_close_timer_->disableTimer(); | ||
| } | ||
| disableDelayedCloseTimer(); |
There was a problem hiding this comment.
Can the timer ever exist here? I don't think so? Can this just be an ASSERT that the timer is nullptr? If so we can remove the private function I think?
There was a problem hiding this comment.
The timer disablement was here as a defensive measure (like the close() call after it is). However, now that closeSocket() is disabling the timer, I agree this is no longer needed and have added a check to the ASSERT().
| } | ||
|
|
||
| // Test that tearing down the connection will disable the delayed close timer. | ||
| TEST_P(ConnectionImplTest, DelayedCloseTimeoutDisableOnSocketClose) { |
There was a problem hiding this comment.
This might be a big pain, but is it possible to switch all these new tests over to using InSequence? For tests like this IMO it makes the tests much stronger.
There was a problem hiding this comment.
Agreed and done.
Signed-off-by: Andres Guedez <aguedez@google.com>
Signed-off-by: Andres Guedez <aguedez@google.com>
|
@AndresGuedez sorry looks like you need another master merge. |
…eout-segv Signed-off-by: Andres Guedez <aguedez@google.com>
Signed-off-by: Andres Guedez <aguedez@google.com>
Done. Thanks! |
Signed-off-by: Andres Guedez <aguedez@google.com>
…ix (envoyproxy#4587) Re-enable the changes reverted in 9d32e5c, which were originally merged as part of envoyproxy#4382. Signed-off-by: Andres Guedez <aguedez@google.com> Signed-off-by: Aaltan Ahmad <aa@stripe.com>
2 participants
Reintroduce PR #4382 (aa9478f) with a bug fix to prevent segfaults during connection tear down race conditions.
#4382 was reverted in PR #4581 (9d32e5c) due to detection of the crashing bug.
Risk Level: Medium (reintroduces medium risk, previously reverted PR)
Testing: Added unit tests to reproduce (previously) crashing states
Docs Changes: N/A
Release Notes: Added
Fixes #4583