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

HTTP/2 Child Channel reading and flushing #7344

Closed
wants to merge 1 commit into
base: 4.1
from

Conversation

@Scottmitch
Member

Scottmitch commented Oct 26, 2017

Motivation:
If a child channel's read is triggered outside the parent channel's read
loop then it is possible a WINDOW_UPDATE will be written, but not
flushed.
If a child channel's beginRead processes data from the inboundBuffer and
then readPending is set to false, which will result in data not being
delivered if in the parent's read loop and more data is attempted to be
delievered to that child channel.

Modifications:

  • The child channel must force a flush if a frame is written as a result
    of reading a frame, and this is not in the parent channel's read loop
  • The child channel must allow a transition from dequeueing from
    beginRead into the parent channel's read loop to deliver more data

Result:
The child channel flushes data when reading outside the parent's read
loop, and has frames delivered more reliably.

HTTP/2 Child Channel reading and flushing
Motivation:
If a child channel's read is triggered outside the parent channel's read
loop then it is possible a WINDOW_UPDATE will be written, but not
flushed.
If a child channel's beginRead processes data from the inboundBuffer and
then readPending is set to false, which will result in data not being
delivered if in the parent's read loop and more data is attempted to be
delievered to that child channel.

Modifications:
- The child channel must force a flush if a frame is written as a result
of reading a frame, and this is not in the parent channel's read loop
- The child channel must allow a transition from dequeueing from
beginRead into the parent channel's read loop to deliver more data

Result:
The child channel flushes data when reading outside the parent's read
loop, and has frames delivered more reliably.

@Scottmitch Scottmitch added the defect label Oct 26, 2017

@Scottmitch Scottmitch added this to the 4.1.17.Final milestone Oct 26, 2017

@Scottmitch Scottmitch self-assigned this Oct 26, 2017

@Scottmitch

This comment has been minimized.

Show comment
Hide comment
@Scottmitch

Scottmitch Oct 26, 2017

Member

Unit test will come as a followup PR.

Member

Scottmitch commented Oct 26, 2017

Unit test will come as a followup PR.

allocHandle.readComplete();
pipeline().fireChannelReadComplete();
if (continueReading && parentReadInProgress) {

This comment has been minimized.

@netkins

netkins Oct 26, 2017

BLOCKER Change this condition so that it does not always evaluate to "false" rule

@netkins

netkins Oct 26, 2017

BLOCKER Change this condition so that it does not always evaluate to "false" rule

* @return {@code true} if a frame has been written as a result of this method call.
* @throws Http2Exception If this operation violates the flow control limits.
*/
boolean onBytesConsumed(@SuppressWarnings("unused") ChannelHandlerContext ctx,

This comment has been minimized.

@netkins

netkins Oct 26, 2017

MAJOR Remove this unused method parameter "ctx". rule

@netkins

netkins Oct 26, 2017

MAJOR Remove this unused method parameter "ctx". rule

}
doRead0((Http2Frame) m, allocHandle);
} while (allocHandle.continueReading());
} while (continueReading = allocHandle.continueReading());

This comment has been minimized.

@netkins

netkins Oct 26, 2017

MAJOR Extract the assignment out of this expression. rule

@netkins

netkins Oct 26, 2017

MAJOR Extract the assignment out of this expression. rule

@Scottmitch

This comment has been minimized.

Show comment
Hide comment
@Scottmitch
Member

Scottmitch commented Oct 26, 2017

4.1 (911b2ac)

@Scottmitch Scottmitch closed this Oct 26, 2017

@Scottmitch Scottmitch deleted the Scottmitch:http2_child_channel_flush branch Oct 26, 2017

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