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.

Sign up for GitHub

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

Jump to bottom

Handle EOS events during server open #11346

Merged
merged 1 commit into from
May 20, 2024
Merged

Handle EOS events during server open #11346

merged 1 commit into from
May 20, 2024

Conversation

duke8253
Copy link
Contributor

We saw a bunch of aborts happening due to EOS not being handled during HttpSM::state_http_server_open, and after some discussion we believe this should be handled the same as a VC_EVENT_ERROR case. This is now deployed globally for us.

@duke8253 duke8253 added this to the 10.1.0 milestone May 13, 2024
@duke8253 duke8253 self-assigned this May 13, 2024
@bryancall bryancall requested a review from cmcfarlen May 13, 2024 22:07
@cmcfarlen
Copy link
Contributor

Other places with VC_EVENT_ERROR but not VC_EVENT_EOS

src/iocore/net/QUICNextProtocolAccept.cc|44 col 8| case VC_EVENT_ERROR:
src/proxy/http/HttpSM.cc|1232 col 8| case VC_EVENT_ERROR:
src/proxy/http/HttpSM.cc|1808 col 8| case VC_EVENT_ERROR:
src/proxy/Transform.cc|634 col 10| case VC_EVENT_ERROR:
src/proxy/Transform.cc|793 col 10| case VC_EVENT_ERROR:

cmcfarlen
cmcfarlen approved these changes May 15, 2024
View reviewed changes
Copy link
Contributor

@cmcfarlen cmcfarlen left a comment

Choose a reason for hiding this comment

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

I do wonder if these other places should handle EOS, but this PR looks fine.

@cmcfarlen cmcfarlen mentioned this pull request May 15, 2024
Closed
@duke8253 duke8253 merged commit f8ad3f9 into apache:master May 20, 2024
15 checks passed
bneradt added a commit to bneradt/trafficserver that referenced this pull request May 20, 2024
@bneradt
Handle VC_EVENT_EOS in more places
d59cc31 
This adds the handling of VC_EVENT_EOS to a few handlers that did handle
VC_EVENT_ERROR but not EOS. This is a follow up from a review comment in apache#11346.
@bneradt bneradt added this to In progress in 9.2.x Branch and Release via automation May 20, 2024
@bneradt bneradt mentioned this pull request May 20, 2024
Merged
bneradt added a commit that referenced this pull request May 20, 2024
@bneradt
Handle VC_EVENT_EOS in more places (#11368)
abc4d5d 
This adds the handling of VC_EVENT_EOS to a few handlers that did handle VC_EVENT_ERROR but not EOS. This is a follow up from a review comment in #11346. See:

#11346 (comment)
@cmcfarlen cmcfarlen modified the milestones: 10.1.0, 10.0.0 May 24, 2024
@cmcfarlen
Copy link
Contributor

Cherry-picked to v10.0.x

cmcfarlen pushed a commit that referenced this pull request May 24, 2024
@duke8253 @cmcfarlen
handle EOS events during server open (#11346)
3c3e624 
(cherry picked from commit f8ad3f9)
cmcfarlen pushed a commit that referenced this pull request May 24, 2024
@bneradt @cmcfarlen
Handle VC_EVENT_EOS in more places (#11368)
e2b7f2c 
This adds the handling of VC_EVENT_EOS to a few handlers that did handle VC_EVENT_ERROR but not EOS. This is a follow up from a review comment in #11346. See:

#11346 (comment)

(cherry picked from commit abc4d5d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmcfarlen cmcfarlen cmcfarlen approved these changes

Assignees

@duke8253 duke8253

Labels
Core Crash
Projects
9.2.x Branch and Release
  
In progress
ATS v10.0.x
Status: picked-10.0.0
Milestone
10.0.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@duke8253 @cmcfarlen