Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

quiche: do not arm stream_idle_timer_ if the stream is closed #22689

Merged
merged 17 commits into from
Aug 17, 2022

Conversation

danzh2010
Copy link
Contributor

@danzh2010 danzh2010 commented Aug 13, 2022

Commit Message: stream_idle_timer_ is armed to timeout the sending of the bufferred response payload in the quic stream send buffer after the end stream is buffered in the stream. But today this timer is armed even if the the encoding of the payload causes the stream to be closed, in which case the timer can never be cancelled till the stream destruction with ASSERT hit as below:

[2022-08-12 22:23:38.843][12][critical][backtrace] [./source/server/backtrace.h:104] Caught Aborted, suspect faulting address 0x50e8d0000000c
[2022-08-12 22:23:38.844][12][critical][backtrace] [./source/server/backtrace.h:91] Backtrace (use tools/stack_decode.py to get line numbers):
[2022-08-12 22:23:38.844][12][critical][backtrace] [./source/server/backtrace.h:92] Envoy version: 0/1.24.0-dev/test/DEBUG/BoringSSL
[2022-08-12 22:23:38.858][12][critical][backtrace] [./source/server/backtrace.h:96] #0: Envoy::SignalAction::sigHandler() [0x3480b28]
[2022-08-12 22:23:38.858][12][critical][backtrace] [./source/server/backtrace.h:96] #1: __restore_rt [0x7f94b072c200]
[2022-08-12 22:23:38.872][12][critical][backtrace] [./source/server/backtrace.h:96] #2: Envoy::Quic::EnvoyQuicStream::~EnvoyQuicStream() [0x2a2fe98]
[2022-08-12 22:23:38.885][12][critical][backtrace] [./source/server/backtrace.h:96] #3: Envoy::Quic::EnvoyQuicServerStream::~EnvoyQuicServerStream() [0x2a78058]
[2022-08-12 22:23:38.899][12][critical][backtrace] [./source/server/backtrace.h:96] #4: Envoy::Quic::EnvoyQuicServerStream::~EnvoyQuicServerStream() [0x2a77d30]
[2022-08-12 22:23:38.912][12][critical][backtrace] [./source/server/backtrace.h:96] #5: Envoy::Quic::EnvoyQuicServerStream::~EnvoyQuicServerStream() [0x2a77d69]

This change check stream close in this case, so that the idle timer will not be armed for closed streams.

Risk Level: low
Testing: new unit test
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

Signed-off-by: Dan Zhang <[email protected]>
Signed-off-by: Dan Zhang <[email protected]>
Signed-off-by: Dan Zhang <[email protected]>
@jmarantz
Copy link
Contributor

/assign-from @envoyproxy/first-pass-reviewers

@repokitteh-read-only
Copy link

@envoyproxy/first-pass-reviewers assignee is @tonya11en

🐱

Caused by: a #22689 (comment) was created by @jmarantz.

see: more, trace.

Copy link
Contributor

@RyanTheOptimist RyanTheOptimist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thanks for doing this

@RyanTheOptimist RyanTheOptimist merged commit bb58560 into envoyproxy:main Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants