-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fix reentrant subscribe in StreamingNettyByteBody #11051
Conversation
If a subscriber to a split streaming body writes a response when reading data from the body, this can ultimately lead to another split of the same body being closed. Closing the body in turn is a subscribe operation, which must not happen during a read in a reentrant fashion. This would lead to a failure in the assertion that guards against such reentrant operations (`assert !working;`), and various downstream issues like buffer leaks. In particular, `MaxRequestSizeSpec` was affected by this bug occasionally. This patch replaces the use of EventLoopFlow with more suitable code. In particular, EventLoopFlow does not support reentrant or concurrent calls, it only ensures serialization. The new logic supports reentrant or concurrent calls and still ensures serialization where it matters. The new test does not work yet due to netty/netty#13730 . This PR is a draft until that patch is released.
@yawkat now that the netty upgrade is done can this proceed? |
Sure I'll update |
Quality Gate failedFailed conditions |
@yawkat Hi, it this issue merged to 4.7? I am using 4.7 and reproduce this issue |
This is fixed in 4.7, yes. You are likely seeing a different issue. |
Maybe it's a similar exception.
|
looks like a bug, please make a new issue. |
Thanks for the reply, I created a new issue. (#11364) |
If a subscriber to a split streaming body writes a response when reading data from the body, this can ultimately lead to another split of the same body being closed. Closing the body in turn is a subscribe operation, which must not happen during a read in a reentrant fashion. This would lead to a failure in the assertion that guards against such reentrant operations (
assert !working;
), and various downstream issues like buffer leaks. In particular,MaxRequestSizeSpec
was affected by this bug occasionally.This patch replaces the use of EventLoopFlow with more suitable code. In particular, EventLoopFlow does not support reentrant or concurrent calls, it only ensures serialization. The new logic supports reentrant or concurrent calls and still ensures serialization where it matters.
The new test does not work yet due to netty/netty#13730 . This PR is a draft until that patch is released.