Bug 66602 - TCP abnormal shutdown during pressure testing based on HTTP2 (h2c)
Summary: TCP abnormal shutdown during pressure testing based on HTTP2 (h2c)
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 9
Classification: Unclassified
Component: Connectors (show other bugs)
Version: 9.0.75
Hardware: PC All
: P1 normal (vote)
Target Milestone: -----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-05-16 10:07 UTC by ledefe
Modified: 2023-05-19 06:11 UTC (History)
0 users



Attachments
normal request log comparison (177.15 KB, image/png)
2023-05-16 10:07 UTC, ledefe
Details

Note You need to log in before you can comment on or make changes to this bug.
Description ledefe 2023-05-16 10:07:52 UTC
Created attachment 38559 [details]
normal request log comparison

**Perform a simple GET request pressure test on the HTTP2 (h2c) service, and the TCP link will shutdown abnormally.**


1.By simulating 50 concurrent requests with 500 each, perform pressure testing and packet capture on a simple GET request.
2.The process found that the TCP link would be disconnected.
3.The reason is that tomcat sent a WINDOW_UPDATE Frame with window size value is zero.


call Http2UpgradeHandler.endRequestBodyFrame method the dataLength is zero? Is this reasonable?

Http2UpgradeHandler:

    public void endRequestBodyFrame(int streamId, int dataLength) throws Http2Exception, IOException {
        AbstractNonZeroStream abstractNonZeroStream = getAbstractNonZeroStream(streamId, true);
        if (abstractNonZeroStream instanceof Stream) {
            ((Stream) abstractNonZeroStream).getInputBuffer().onDataAvailable();
        } else {
            // The Stream was recycled between the call in Http2Parser to
            // startRequestBodyFrame() and the synchronized block that contains
            // the call to this method. This means the bytes read will have been
            // written to the original stream and, effectively, swallowed.
            // Therefore, need to notify that those bytes were swallowed here.
            onSwallowedDataFramePayload(streamId, dataLength);
        }
    }
Comment 1 ledefe 2023-05-16 10:23:16 UTC
Comment on attachment 38559 [details]
normal request log comparison

The overall process is as follows:

1. http-nio-9988-exec-4 thread: Read data from the stream. After reading the header frame, asynchronously start the request distribution processing for data (http-nio-9988-exec-13) and continue reading the data frame data.

2. http-nio-9988-exec-13 thread: Process data requests. If the request is completed, the status changes to CLOSE and replaces the current stream with the recycling stream(RecycledStream)

3. http-nio-9988-exec-4 thread: Process data frame data. If it is processed, execute endRequestBodyFrame. is RecycledStream will send WINDOW_UPDATE notify.
Comment 2 ledefe 2023-05-18 05:45:35 UTC
Didn't anyone reply?
Comment 3 Mark Thomas 2023-05-18 09:49:49 UTC
If you want a guaranteed SLA, you'll have to pay for it.

I can't speak for the other committers but I have been preparing for a conference this week. I was planning on looking at this next week unless someone else gets there first.

This will get looked at (and fixed if necessary) between now and the June releases.
Comment 4 Mark Thomas 2023-05-18 11:55:08 UTC
As per RFC 9113, section 6.9 it is not legal to send a WINDOW_UPDATE frame of length zero.

I've had a quick look at the code it should be a simple fix to skip sending the WINDOW_UPDATE frame if the increment is zero.
Comment 5 Mark Thomas 2023-05-18 14:10:44 UTC
Could you be tempted into submitting a patch or PR for this? (Don't forget the change log.)
Comment 6 ledefe 2023-05-19 05:37:40 UTC
A PR has been submitted based on the 9.0.x branch 

PR: https://github.com/apache/tomcat/pull/619
Comment 7 Han Li 2023-05-19 06:11:29 UTC
Thanks for the report and PR.

Fixed in:
- 11.0.x for 11.0.0-M7 onwards
- 10.1.x for 10.1.10 onwards
- 9.0.x for 9.0.76 onwards
- 8.5.x for 8.5.90 onwards