Skip to content

Conversation

qingdaoheze
Copy link

The request statistics will be wrong when Request object is reused in the object pool.

@rmaucher
Copy link
Contributor

Are you trying to fix a problem you noticed, or is this PR based on code analysis ?

@rmaucher rmaucher closed this Jun 12, 2025
@qingdaoheze
Copy link
Author

Sorry. I don't understand what you mean. My issue is that the metrics in org.apache.coyote.RequestGroupInfo#removeRequestProcessor is not counted correctly when org.apache.coyote.http2.Http2Protocol#discardRequestsAndResponses is false(default). This PR can resolve this issue partially, because there is still some special scenarios that miss some count. I hope you can give an better solution.
@rmaucher

@rmaucher
Copy link
Contributor

Definitely this PR is wrong. RequestInfo is not the stats for an individual request, so it should not be reset.

@markt-asf
Copy link
Contributor

HTTP/2 is handled differently because of the multiplexing. We probably do need a recycle() method on RequestInfo. It looks like it should be called around line 440 (current 9.0.x code) of StreamProcessor.

@qingdaoheze
Copy link
Author

image

@rmaucher When the org.apache.coyote.Request pool is used, its org.apache.coyote.Request#reqProcessorMX is not recycled after the request is recycled. So when org.apache.coyote.RequestGroupInfo#removeRequestProcessor is run, its stat count is counted many times. So the result count in org.apache.coyote.RequestGroupInfo will be wrong.

@markt-asf markt-asf reopened this Jun 16, 2025
@rmaucher
Copy link
Contributor

HTTP/2 is handled differently because of the multiplexing. We probably do need a recycle() method on RequestInfo. It looks like it should be called around line 440 (current 9.0.x code) of StreamProcessor.

That looks correct, you know that code better than I do.

@qingdaoheze
Copy link
Author

qingdaoheze commented Jun 17, 2025

HTTP/2 is handled differently because of the multiplexing. We probably do need a recycle() method on RequestInfo. It looks like it should be called around line 440 (current 9.0.x code) of StreamProcessor.

@markt-asf For this solution, there is also a concurrency problem. Because the request has already returned to the pool on the line 150 of StreamProcessor. Then the recycle method is executed on line 151 of StreamProcessor. In the progress, the request has been returned to the pool may be borrowed by another request and its reqProcessorMX may has been changed.
image

@markt-asf
Copy link
Contributor

Fixed in 4dc1a0f and back-ported

@markt-asf markt-asf closed this Jun 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants