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

Prevent vapor streaming requests from randomly losing a first chunk, stalling the request #2917

Merged
merged 6 commits into from Dec 16, 2022

Conversation

Joannis
Copy link
Member

@Joannis Joannis commented Nov 20, 2022

Fixes a bug where a streaming body from a request was not processing a chunk, causing the request to stall. This happened when a handler was being set at the exact moment a chunk was being processed, causing the chunk to be added to a now irrelevant array that was just processed while switching to a streaming callback.

See #2906

@Joannis Joannis requested a review from 0xTim November 20, 2022 23:06
@0xTim
Copy link
Member

0xTim commented Nov 27, 2022

@Joannis still got test failures

@0xTim 0xTim linked an issue Nov 27, 2022 that may be closed by this pull request
@vzsg
Copy link
Member

vzsg commented Nov 27, 2022

Deadlocks are a b—

@Joannis
Copy link
Member Author

Joannis commented Nov 28, 2022

Haven't had the time to find a better solution yet due to time constraints

@Joannis Joannis requested a review from gwynne December 10, 2022 03:20
@Joannis Joannis added the semver-patch Internal changes only label Dec 10, 2022
if self.eventLoop.inEventLoop {
write0(chunk, promise: promise)
} else {
self.eventLoop.execute {
Copy link
Member

Choose a reason for hiding this comment

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

It is my understanding that EventLoop.execute(_:) already does the "is in event loop" check internally, which in addition, can return false negatives in the normal course of execution - If the problem is that you were executing on the event loop unconditionally before and are skipping the extra "hop", this isn't a guaranteed solution.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not supposed to be a guaranteed solution either. The main reason is that I think it's a waste of a hop. And it does always queue it for the next tick. Not only does it fix the test, it also is a good optimisation to have.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally, the test's assumption is to be fixed as well of course.

@Joannis Joannis requested a review from gwynne December 10, 2022 07:27
Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

Thanks!

@0xTim 0xTim merged commit eb2da0d into main Dec 16, 2022
@0xTim 0xTim deleted the feature/jo-lost-chunks branch December 16, 2022 23:05
@VaporBot
Copy link
Contributor

These changes are now available in 4.67.5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch Internal changes only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Collecting the request body sometimes hangs
5 participants