Repository navigation
transport-netty-internal: don't leave cancelled, partially written client requests open - #3669
bryce-anderson wants to merge 1 commit into
Conversation
…ient requests open #### Motivation Cancelling a client write cancelled only the request body's source, so the transport never ended a partially written request. On HTTP/2, a stream cancelled after its response finished but before its request body did was never reset: the server kept waiting for the rest of the body, and the stream held a concurrent-stream slot on both peers until the server reset it or the connection closed. Enough of these made the client open extra connections. #### Modifications - A client write cancelled after part of the request was written now closes the outbound side: HTTP/2 resets the stream, and HTTP/1.x shuts down output. Writes that finished or never started, and server writes, are unaffected. #### Result A cancelled client request no longer stays open after it was partially written. Behavior change: an HTTP/2 server now receives RST_STREAM(CANCEL) for a client stream cancelled while its request body is still being written, including when the response has already finished. No action is needed.
| if (eventLoop.inEventLoop()) { | ||
| promise.sourceCancelled(); | ||
| } else { | ||
| eventLoop.execute(promise::sourceCancelled); |
There was a problem hiding this comment.
When the cancel comes from another thread, sourceCancelled is queued, but DefaultNettyConnection has already pointed channelOutboundListener back at the connection. If the last chunk or trailers write was already queued, its OutboundDataEndEvent goes to the connection's no-op listener instead of this subscriber. sourceCancelled then treats a fully written request as partial and closes outbound. On HTTP/1.1 that half-closes a keep-alive connection that should have been reusable. On HTTP/2 a stream that already completed gets an unnecessary RST.
| if (written) { | ||
| // Part of the request is on the wire and nothing can end it cleanly. Closing outbound lets the peer | ||
| // see the truncation, and keeps the next request from being written after it. | ||
| closeHandler.closeChannelOutbound(channel); |
There was a problem hiding this comment.
If we consider a recent case of HTTP/1.1 with pipelining, then it doesn't wait for reading responses of earlier pipelined requests that were fully written, like NettyPipelinedConnection.readWithTurn waits for its turn. Remote peers that do not support half-closed connections will close entire TCP channel affecting other responses.
| } | ||
| subscriber.onSubscribe(concurrentSubscription); | ||
| subscriber.onSubscribe(isClient ? () -> { | ||
| sourceCancelled(); |
There was a problem hiding this comment.
When this runs on the event loop, we can cancel twice: if an HTTP/2 write is stalled by flow control and the response is done, closing the stream fails that write. setFailure0 then cancels the subscription and calls onError from inside cancel(), and the lambda cancels it again. As a result writeCancelled() fires twice, which breaks the FlushStrategy rule of at most one call, and observers see ClosedChannelException for what was a user cancel.
| CountDownLatch responseRead = new CountDownLatch(1); | ||
| CountDownLatch streamClosed = new CountDownLatch(1); | ||
| CompletableFuture<Throwable> serverRequestBodyError = new CompletableFuture<>(); | ||
| try (ServerContext server = HttpServers.forAddress(localAddress(0)).protocols(h2Default()) |
There was a problem hiding this comment.
Would be great to test HTTP/1.1 scenarios as well. IIUC, before your fix the server can read the next request as payload of the previous one
| } | ||
|
|
||
| @Test | ||
| void clientCancelAfterPartialWriteClosesOutbound() { |
There was a problem hiding this comment.
Consider testing some of these scenarios in WriteStreamSubscriberOutOfEventloopTest to provide coverage for both paths
| return; | ||
| } | ||
| subscriber.onSubscribe(concurrentSubscription); | ||
| subscriber.onSubscribe(isClient ? () -> { |
There was a problem hiding this comment.
Optional: wdyt if instead of allocating a captured lambda here we implement Cancellable interface and reuse current test-only cancel() method?
final class WriteStreamSubscriber implements PublisherSource.Subscriber<Object>, ChannelOutboundListener,
Cancellable {
...
subscriber.onSubscribe(isClient ? this : concurrentSubscription);
...
@Override
public void cancel() {
Subscription oldVal = subscriptionUpdater.getAndSet(this, CANCELLED);
if (oldVal == null || oldVal == CANCELLED) {
return;
}
if (eventLoop.inEventLoop()) {
cancel0(oldVal);
} else {
eventLoop.execute(() -> cancel0(oldVal));
}
}
private void cancel0(Subscription oldVal) {
if (isClient) {
promise.sourceCancelled();
}
oldVal.cancel();
}It may also help to fix "cancel twice" problem bcz of subscriptionUpdater and skips work after a finished source.
| } | ||
|
|
||
| @Override | ||
| public void terminateSource() { |
There was a problem hiding this comment.
Most likely for a follow-up:
There is a similar leak possible with 100-continue use-case. If server responds with 417, HTTP/2 stream stays half-open and keeps its concurrent-stream slot. This method is triggered by CancelWriteUserEvent path.
|
I may have found a better way forward, but leaving this available for now because it's not certain. |
|
Alternative PR here: #3678. |
This branch has not been deployed
2 participants
Motivation
Cancelling a client write cancelled only the request body's source, so the transport never ended a partially written request. On HTTP/2, a stream cancelled after its response finished but before its request body did was never reset: the server kept waiting for the rest of the body, and the stream held a concurrent-stream slot on both peers until the server reset it or the connection closed. Enough of these made the client open extra connections.
Modifications
Result
A cancelled client request no longer stays open after it was partially written.
Behavior change: an HTTP/2 server now receives RST_STREAM(CANCEL) for a client stream cancelled while its request body is still being written, including when the response has already finished. No action is needed.