rschmitt commented on code in PR #543:
URL:
https://github.com/apache/httpcomponents-core/pull/543#discussion_r2263801573
##########
httpcore5/src/main/java/org/apache/hc/core5/http/impl/nio/AbstractHttp1StreamDuplexer.java:
##########
@@ -277,13 +278,14 @@ public final void onInput(final ByteBuffer src) throws
HttpException, IOExceptio
return;
}
- boolean endOfStream = false;
if (incomingMessage == null) {
final int bytesRead = inbuf.fill(ioSession);
Review Comment:
I ran my tests against your change set. Here are my findings:
1. Uncontended connection reuse is moderately more reliable with HTTP, but
not with HTTPS.
2. Contended connection reuse is the same, with failure rates of 67% for
small batches and 50% for large batches.
I've pushed my reproducer
[here](https://github.com/rschmitt/httpcomponents-client/blob/race/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/TestConnectionClosureRace.java)
so you can experiment with it. One thing I quickly discovered is that your
change set is ineffective on HTTPS because `SSLIOSession::read` is just
returning the same cached value every time:
```
@Override
public int read(final ByteBuffer dst) {
return endOfStream ? -1 : 0;
}
```
This is altogether different from HTTP, where I've confirmed that the
duplexer reads `endOfStream` on the second call to `fillBuffer()`.
To me, the biggest mystery is why contended connection reuse is failing at
such a consistently high rate:
```
Core version: 5.4-alpha1-SNAPSHOT
Client version: 5.6-alpha1-SNAPSHOT
Http: Sequential requests (rapid): 2,496 succeeded; 4 failed (99.84% success
rate)
Http: Sequential requests (slow): 10 succeeded; 0 failed (100.00% success
rate)
Http: Single large batch: 15 succeeded; 15 failed (50.00% success rate)
Http: Multiple small batches: 10 succeeded; 5 failed (66.67% success rate)
Https: Sequential requests (rapid): 2,235 succeeded; 265 failed (89.40%
success rate)
Https: Sequential requests (slow): 10 succeeded; 0 failed (100.00% success
rate)
Https: Single large batch: 15 succeeded; 15 failed (50.00% success rate)
Https: Multiple small batches: 10 succeeded; 5 failed (66.67% success rate)
```
One fascinating detail is that your change set caused the HTTP batch tests
to see `RequestNotExecutedException`, where previously they saw
`ConnectionClosedException`. Before:
```
Http: Sequential requests (rapid): 2,479 succeeded; 21 failed (99.16%
success rate)
14 not executed, 6 closed, 0 reset, 1 other
Http: Sequential requests (slow): 10 succeeded; 0 failed (100.00% success
rate)
0 not executed, 0 closed, 0 reset, 0 other
Http: Single large batch: 15 succeeded; 15 failed (50.00% success rate)
0 not executed, 15 closed, 0 reset, 0 other
Http: Multiple small batches: 10 succeeded; 5 failed (66.67% success rate)
0 not executed, 5 closed, 0 reset, 0 other
```
After:
```
Http: Sequential requests (rapid): 2,491 succeeded; 9 failed (99.64% success
rate)
9 not executed, 0 closed, 0 reset, 0 other
Http: Sequential requests (slow): 10 succeeded; 0 failed (100.00% success
rate)
0 not executed, 0 closed, 0 reset, 0 other
Http: Single large batch: 15 succeeded; 15 failed (50.00% success rate)
15 not executed, 0 closed, 0 reset, 0 other
Http: Multiple small batches: 10 succeeded; 5 failed (66.67% success rate)
5 not executed, 0 closed, 0 reset, 0 other
```
This must mean that the connection is being returned to the pool the very
instant the HTTP response is read, before the second `fillBuffer()` call takes
place (or it means that the new `endOfStream` check is ineffective).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
