Skip to content

HTTP/2 should forward shutdown user events to active streams#13394

Merged
normanmaurer merged 2 commits into
netty:4.1from
idelpivnitskiy:h2-stream-shutdown
May 23, 2023
Merged

HTTP/2 should forward shutdown user events to active streams#13394
normanmaurer merged 2 commits into
netty:4.1from
idelpivnitskiy:h2-stream-shutdown

Conversation

@idelpivnitskiy

@idelpivnitskiy idelpivnitskiy commented May 22, 2023

Copy link
Copy Markdown
Member

Motivation:

The following 4 user events mean that one of the sides of the parent channel is shutdown. Http2MultiplexHandler and Http2MultiplexCodec should propagate those events to all active streams to let them identify if they still can complete or not. For example, streams that didn't receive endStream flag can decide to close if they observe one of these events. These events should still propagate forward through the parent channel pipeline.

Modifications:

  • Create Http2FrameStreamVisitors for ChannelInputShutdownEvent, ChannelInputShutdownReadComplete, ChannelOutputShutdownEvent, SslCloseCompletionEvent;
  • Propagate these events to all active streams of Http2MultiplexHandler and Http2MultiplexCodec;
  • Add a test to validate new behavior;

Result:

Active child streams can observe when the parent channel's input or output is shutdown or if it received close_notify.

Motivation:

The following 4 user events mean that one of the sides of the parent
channel is shutdown. `Http2MultiplexHandler` should propagate those
events to all active streams to let them identify if they still can
complete or not. For example, streams that didn't receive `endStream`
flag can decide to close if they observe one of these events.
These events should still propagate through the parent channel pipeline.

Modifications:
- Create `Http2FrameStreamVisitor`s for `ChannelInputShutdownEvent`,
`ChannelInputShutdownReadComplete`, `ChannelOutputShutdownEvent`,
`SslCloseCompletionEvent`;
- Propagate these events to all active streams;
- Add a test to validate new behavior;

Result:

Active child streams can observe when parent channel's input or
output is shutdown or if it received `close_notify`.
@normanmaurer normanmaurer added this to the 4.1.93.Final milestone May 22, 2023
@idelpivnitskiy idelpivnitskiy changed the title Http2MultiplexHandler forward some user events to active streams HTTP/2 should forward shutdown user events to active streams May 22, 2023
@normanmaurer normanmaurer merged commit 3d8ff07 into netty:4.1 May 23, 2023
@normanmaurer

Copy link
Copy Markdown
Member

@idelpivnitskiy thanks a lot!

@normanmaurer

Copy link
Copy Markdown
Member

Need to think about how to do something like this in 5.0 as well.

Veryfirefly pushed a commit to Veryfirefly/netty that referenced this pull request May 23, 2023
…3394)

Motivation:

The following 4 user events mean that one of the sides of the parent
channel is shutdown. `Http2MultiplexHandler` and `Http2MultiplexCodec`
should propagate those events to all active streams to let them identify
if they still can complete or not. For example, streams that didn't
receive `endStream` flag can decide to close if they observe one of
these events. These events should still propagate forward through the
parent channel pipeline.

Modifications:
- Create `Http2FrameStreamVisitor`s for `ChannelInputShutdownEvent`,
`ChannelInputShutdownReadComplete`, `ChannelOutputShutdownEvent`,
`SslCloseCompletionEvent`;
- Propagate these events to all active streams of
`Http2MultiplexHandler` and `Http2MultiplexCodec`;
- Add a test to validate new behavior;

Result:

Active child streams can observe when the parent channel's input or
output is shutdown or if it received `close_notify`.
@idelpivnitskiy idelpivnitskiy deleted the h2-stream-shutdown branch May 23, 2023 19:45
idelpivnitskiy added a commit to idelpivnitskiy/netty that referenced this pull request May 23, 2023
Motivation:

Follow-up for netty#13394 to remove `ChannelInputShutdownEvent` propagation.
Discussed with @Scottmitch, `ChannelInputShutdownEvent` is a premature
event, most users should rely on `ChannelInputShutdownReadComplete`
instead. Let's propagate only the later one and if there is a use-case
for `ChannelInputShutdownEvent` we can add its propagation later.

Modifications:

- Remove `CHANNEL_INPUT_SHUTDOWN_EVENT_VISITOR` to stop propagation of
`ChannelInputShutdownEvent`;
- Adjust tests;

Result:

`ChannelInputShutdownEvent` is not propagated for now. We can add it
back when there is a use-case.
Scottmitch pushed a commit that referenced this pull request May 24, 2023
…13395)

Motivation:

Follow-up for #13394 to remove `ChannelInputShutdownEvent` propagation.
Discussed with @Scottmitch, `ChannelInputShutdownEvent` is a premature
event, most users should rely on `ChannelInputShutdownReadComplete`
instead. Let's propagate only the later one and if there is a use-case
for `ChannelInputShutdownEvent` we can add its propagation later.

Modifications:

- Remove `CHANNEL_INPUT_SHUTDOWN_EVENT_VISITOR` to stop propagation of
`ChannelInputShutdownEvent`;
- Adjust tests;

Result:

`ChannelInputShutdownEvent` is not propagated for now. We can add it
back when there is a use-case.
dongjoon-hyun pushed a commit to apache/spark that referenced this pull request Jun 28, 2023
### What changes were proposed in this pull request?
This pr aims to upgrade netty from 4.1.92 to 4.1.93.

### Why are the changes needed?
1.v4.1.92 VS v4.1.93
netty/netty@netty-4.1.92.Final...netty-4.1.93.Final

2.The new version brings some bug fix, eg:
- Reset byte buffer in loop for AbstractDiskHttpData.setContent ([#13320](netty/netty#13320))
- OpenSSL MAX_CERTIFICATE_LIST_BYTES option supported ([#13365](netty/netty#13365))
- Adapt to DirectByteBuffer constructor in Java 21 ([#13366](netty/netty#13366))
- HTTP/2 encoder: allow HEADER_TABLE_SIZE greater than Integer.MAX_VALUE ([#13368](netty/netty#13368))
- Upgrade to latest netty-tcnative to fix memory leak ([#13375](netty/netty#13375))
- H2/H2C server stream channels deactivated while write still in progress ([#13388](netty/netty#13388))
- Channel#bytesBefore(un)writable off by 1 ([#13389](netty/netty#13389))
- HTTP/2 should forward shutdown user events to active streams ([#13394](netty/netty#13394))
- Respect the number of bytes read per datagram when using recvmmsg ([#13399](netty/netty#13399))

3.The release notes as follows:
- https://netty.io/news/2023/05/25/4-1-93-Final.html

4.Why not upgrade to `4-1-94-Final` version?
Because the return value of the 'threadCache()' (from `PoolThreadCache` to `PoolArenasCache`) method of the netty Inner class used in the 'arrow memory netty' version '12.0.1' has changed and belongs to break change, let's wait for the upgrade of the 'arrow memory netty' before upgrading to the '4-1-94-Final' version.

The reference is as follows:
https://github.com/apache/arrow/blob/6af660f48472b8b45a5e01b7136b9b040b185eb1/java/memory/memory-netty/src/main/java/io/netty/buffer/PooledByteBufAllocatorL.java#L164
https://github.com/netty/netty/blob/da1a448d5bc4f36cc1744db93fcaf64e198db2bd/buffer/src/main/java/io/netty/buffer/PooledByteBufAllocator.java#L732-L736

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Pass GA.

Closes #41681 from panbingkun/upgrade_netty.

Authored-by: panbingkun <pbk1982@gmail.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants