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

Respect ctx.read() calls while processing reads for the child channel… #8617

Merged
merged 1 commit into from Dec 5, 2018

Conversation

@normanmaurer
Copy link
Member

normanmaurer commented Dec 3, 2018

…s when using the Http2MultiplexCodec.

Motivation:

We did not correct respect ctx.read() calls while processing a read for a child Channel. This could lead to read stales when auto read is disabled and no other read was requested.

Modifications:

  • Keep track of extra read() calls while processing reads
  • Add unit tests that verify that read() is respected when triggered either in channelRead(...) or channelReadComplete(...)

Result:

Fixes #8209.

@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Dec 3, 2018

@rschmitt FYI... this should fix your problem. Thanks again for the reproducer.

@@ -1026,7 +1057,11 @@ void readEOS() {

void notifyReadComplete(Handle allocHandle) {
assert next == null && previous == null;
readInProgress = false;
if (readStatus == ReadStatus.REQUESTED) {
readStatus = ReadStatus.IN_PROGRESS;

This comment has been minimized.

@rschmitt

rschmitt Dec 3, 2018

Are you sure this is all you have to do? If auto-read is disabled on the parent channel, might you not have to request a read to ensure progress? (This may happen somewhere else, but I can't see where.)

This comment has been minimized.

@normanmaurer

normanmaurer Dec 4, 2018

Author Member

@rschmitt that is a different "issue" and I am not even sure if we should do anything here at all by default. You could easily add a ChannelOutboundHandler to the ChannelPipeline of the Channel which will trigger channel.parent().read() when a read is received.

This comment has been minimized.

@normanmaurer

normanmaurer Dec 4, 2018

Author Member

We could also make this a ChannelOption on the "child channel".

This comment has been minimized.

@normanmaurer

normanmaurer Dec 4, 2018

Author Member

Just to give some more background why I think we should not handle this by default... I think the user may want to handle the read pattern / back pressure independently in some cases.

This comment has been minimized.

@rschmitt

rschmitt Dec 4, 2018

If this is the philosophy behind this class, then I have no idea why my current h2 server works, because it only reads from the parent channel one time (right at the end of initChannel), and autoread is constantly disabled on both parent and child streams.

This comment has been minimized.

@normanmaurer

normanmaurer Dec 4, 2018

Author Member

I will investigate once this one is pulled in... Like I said I think these are different "concerns".

This comment has been minimized.

@normanmaurer

normanmaurer Dec 5, 2018

Author Member

@rschmitt now that this one is merged I will check your other concern during this week.

* {@link RecvByteBufAllocator} behavior a read may extend beyond the {@link Http2ChannelUnsafe#beginRead()}
* method scope. The {@link Http2ChannelUnsafe#beginRead()} loop may drain all pending data, and then if the
* parent channel is reading this channel may still accept frames.
* This variable represents if a read is in progress for the current channe or was requested.

This comment has been minimized.

@rschmitt

rschmitt Dec 3, 2018

channe => channel

@@ -757,7 +779,7 @@ void fireChildRead(Http2Frame frame) {
assert eventLoop().inEventLoop();
if (!isActive()) {
ReferenceCountUtil.release(frame);
} else if (readInProgress) {
} else if (readStatus != ReadStatus.IDLE) {
// If readInProgress there cannot be anything in the queue, otherwise we would have drained it from the

This comment has been minimized.

@rschmitt

rschmitt Dec 3, 2018

"If a read is in progress or has been requested, there cannot be anything in the queue" (etc)

Respect ctx.read() calls while processing reads for the child channel…
…s when using the Http2MultiplexCodec.

Motivation:

We did not correct respect ctx.read() calls while processing a read for a child Channel. This could lead to read stales when auto read is disabled and no other read was requested.

Modifications:

- Keep track of extra read() calls while processing reads
- Add unit tests that verify that read() is respected when triggered either in channelRead(...) or channelReadComplete(...)

Result:

Fixes #8209.

@normanmaurer normanmaurer force-pushed the http2_multiplex_read branch from b30f230 to 79816d9 Dec 4, 2018

@normanmaurer normanmaurer added the defect label Dec 4, 2018

@normanmaurer normanmaurer added this to the 4.1.33.Final milestone Dec 4, 2018

@trustin

trustin approved these changes Dec 5, 2018

@normanmaurer normanmaurer merged commit 9f9aa1a into 4.1 Dec 5, 2018

4 checks passed

pull request validation (centos6-java11) Build finished.
Details
pull request validation (centos6-java12) Build finished.
Details
pull request validation (centos6-java8) Build finished.
Details
pull request validation (centos6-java9) Build finished.
Details

@normanmaurer normanmaurer deleted the http2_multiplex_read branch Dec 5, 2018

normanmaurer added a commit that referenced this pull request Dec 5, 2018

Respect ctx.read() calls while processing reads for the child channel…
…s when using the Http2MultiplexCodec. (#8617)

Motivation:

We did not correct respect ctx.read() calls while processing a read for a child Channel. This could lead to read stales when auto read is disabled and no other read was requested.

Modifications:

- Keep track of extra read() calls while processing reads
- Add unit tests that verify that read() is respected when triggered either in channelRead(...) or channelReadComplete(...)

Result:

Fixes #8209.
@carl-mastrangelo

This comment has been minimized.

Copy link
Member

carl-mastrangelo commented Dec 6, 2018

Code LGTM

@@ -783,7 +805,7 @@ void fireChildRead(Http2Frame frame) {

void fireChildReadComplete() {
assert eventLoop().inEventLoop();
assert readInProgress;
assert readStatus == ReadStatus.IN_PROGRESS;

This comment has been minimized.

@bryce-anderson

bryce-anderson Dec 7, 2018

Contributor

Is this still accurate? If the user had requested a read while in the IN_PROGRESS state I think we transition to the REQUESTED state. In fact, the unsafe.notifyReadComplete call right after the assert will check if we're in the REQUESTED state and transition back to IN_PROGRESS.

This comment has been minimized.

@normanmaurer

normanmaurer Dec 7, 2018

Author Member

@bryce-anderson I think you are right... it should be either IN_PROGRESS or REQUESTED. Let me fix this.

This comment has been minimized.

@normanmaurer

normanmaurer Dec 7, 2018

Author Member

@bryce-anderson PTAL #8639 and thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment