Skip to content

HTTPCORE-639: Partially revert expensive early-response checks#210

Closed
carterkozak wants to merge 3 commits intoapache:masterfrom
carterkozak:ckozak/HTTPCORE-639-5.0.x
Closed

HTTPCORE-639: Partially revert expensive early-response checks#210
carterkozak wants to merge 3 commits intoapache:masterfrom
carterkozak:ckozak/HTTPCORE-639-5.0.x

Conversation

@carterkozak
Copy link
Copy Markdown
Contributor

This patch mostly restores the behavior of hc 4.x releases in
which large early responses can result in the client blocking until
a socket exception is thrown. Behavior of plain http detection
is not modified, but only works prior to jre13 due to
http://openjdk.java.net/jeps/353.

This behavior will be configurable in 5.1 releases, but has been
removed as the result of a request entity performance regression
that limited any request to at minimum a full millisecond and at
most 8 MiB/second upload speed regardless of the network.

Carter Kozak and others added 3 commits July 30, 2020 18:25
This patch mostly restores the behavior of hc 4.x releases in
which large early responses can result in the client blocking until
a socket exception is thrown. Behavior of plain http detection
is not modified, but only works prior to jre13 due to
http://openjdk.java.net/jeps/353.

This behavior will be configurable in 5.1 releases, but has been
removed as the result of a request entity performance regression
that limited any request to at minimum a full millisecond and at
most 8 MiB/second upload speed regardless of the network.
@michael-o
Copy link
Copy Markdown
Member

michael-o commented Aug 4, 2020

I am not convinced that this is a good idea. It should either work consistently accross protocols or nothing at all.

@carterkozak
Copy link
Copy Markdown
Contributor Author

I'm conflicted about the backport. On one hand It's helpful to minimize impact in bugfix releases, if we can avoid changing behavior for users without TLS, that could be helpful. On the other hand I agree that it's confusing to support this feature in some non-obvious configurations, it's common to run tests without ssl and we wouldn't want to create a false sense of security.

@michael-o
Copy link
Copy Markdown
Member

I think we have tests for TLS too since ALPN is tested. We can consider the implementation in 5.0.x after this PR as naive as for the SHOULD in the RFC. No guarantees made.

@carterkozak
Copy link
Copy Markdown
Contributor Author

Thanks, I'm closing this because a3870ef has been committed.

@carterkozak carterkozak closed this Aug 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants