Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Address
maxConcurrentStreams violation on write timeout (#3908)
**Background** Currently, armeria maintains a state `HttpResponseDecoder#unfinishedResponses` to check how many in-flight requests are being processed for a connection. Armeria uses this value to check if all connections occupy too many concurrent streams, and creates a new connection if necessary. On the other hand, netty maintains it's own state to check how many in-flight requests are being processed for a connection. (`DefaultHttp2Connection.DefaultEndpoint#numActiveStreams`) Netty checks this value before creating a stream, and throws a `Http2Exception$StreamException` if `MAX_CONCURRENT_STREAMS` is unavailable. **Problem Statement** Currently, when a `WriteTimeoutException` is triggered, armeria decrements `unfinishedResponses` and removes the response. (A `WriteTimeoutException` is thrown when a request header isn't written within a predefined `writeTimeoutMillis`) However, netty may not be aware that armeria has failed the response. Consequently, netty's `numActiveStreams` is greater than armeria's `unfinishedResponses`. This may cause a violation of `MAX_CONCURRENT_STREAMS` for additional requests on the connection. **Motivation** Netty always calls `Http2ResponseDecoder.onStreamClosed` before decrementing `numActiveStreams`. If we want `numActiveStreams` to be in sync with `unfinishedResponses`, I propose that we modify the timing of decrementing `unfinishedResponses` to `Http2ResponseDecoder.onStreamClosed`. In detail, when a `WriteTimeoutException` is scheduled https://github.com/line/armeria/blob/117a21e17ec9e30b0c3c2d74d16fdde3cab62434/core/src/main/java/com/linecorp/armeria/client/HttpRequestSubscriber.java#L171-L173 the response is closed. https://github.com/line/armeria/blob/117a21e17ec9e30b0c3c2d74d16fdde3cab62434/core/src/main/java/com/linecorp/armeria/client/HttpRequestSubscriber.java#L318 Consequently, after the stream processes the `close` event, `whenComplete` is triggered. https://github.com/line/armeria/blob/117a21e17ec9e30b0c3c2d74d16fdde3cab62434/core/src/main/java/com/linecorp/armeria/client/Http2ResponseDecoder.java#L83-L90 And the response is removed (and `unfinishedResponses` is decremented) https://github.com/line/armeria/blob/117a21e17ec9e30b0c3c2d74d16fdde3cab62434/core/src/main/java/com/linecorp/armeria/client/Http2ResponseDecoder.java#L101 However, as far as netty is concerned, the request may have been written and may still be processing. **Misc** Reproduced `maxConcurrentStreams` when `WriteTimeoutException` occurs at 225a684 **Modifications** - Remove the `removeResponse` call from `Http2ResponseDecoder. onWrapperCompleted`, and rely on `onStreamClosed` to remove the response/decrement `unfinishedResponses` - When receiving callbacks for `onHeadersRead`, `onDataRead`, `onRstStreamRead`, also check if `resWrapper` had been closed. This preserves behavior since `res` was previously removed on `WriteTimeoutException`, resulting in `res == null`. *Update* I realized that if we simply don't process values when headers/data/rst are received, then we might not send a `GoAway` and close the connection when `disconnectWhenFinished = true` due to df43379. I've verified this behavior from test cases added in 8018da1 I've modified further such that: - Only remove responses when `onStreamClosed` is called. - Remove calls to `channel().close();` if `shouldSendGoAway()` is true for `onDataRead`, `onHeadersRead` since `onStreamClosed` will handle this instead. - Remove `onStreamClosed` to try to close the `ResponseWrapper` only if the underlying `delegate` is open. d1183d8 There is a slight change of behavior, where a `GoAway` may be triggered from `onRstStream` as well. Let me know if this change shouldn't be made 🙏 Result: - Closes #3858
- Loading branch information
