Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MRESOLVER-584] JDK transport HTTP/2 GOAWAY improvement #532

Merged
merged 4 commits into from
Aug 2, 2024

Conversation

cstamas
Copy link
Member

@cstamas cstamas commented Jul 12, 2024

Changes:

  • drop 21 closer, it abruptly uses "shutdown now"
  • in 11 make httpClient NOT stored per session, but per transport instance
  • simplify and refactor, put things in places like insecure mode config

https://issues.apache.org/jira/browse/MRESOLVER-584

@cstamas cstamas self-assigned this Jul 12, 2024
@cstamas
Copy link
Member Author

cstamas commented Jul 12, 2024

I have a feeling that "swapping" will not even happen. What actually happened in this PR is that HttpClient got much shorter lifespan. Originally, HttpClient was kept and reused (for same session+repository) across whole session. Now, the client shares lifecycle with Transport instance, that is OTOH created by connector per invocation.

@cstamas cstamas changed the title [MRESOLVER-584] Naive attempt for JDK transport [MRESOLVER-584] JDK transport GOAWAY fix Jul 12, 2024
@cstamas
Copy link
Member Author

cstamas commented Jul 13, 2024

This fix is wrong.

Tested using reproducer https://issues.apache.org/jira/browse/MNG-8145 (reused same config and all).
Maven 4.0.0-beta-3 fails (reproducer reproduces). And this PR built JDK transport works OK.

BUT, the reason why it works is due this PR "shortening" the lifespan of JDK HttpClient, and not due "client swap".

When I set nginx config from original keepalive_requests 500; to keepalive_requests 100; (or even 50), this PR fails in very same way.

Still, the PR is correct in a way it keeps HttpClient per Transporter instance, and that is correct change.

Will see in some other solution...

@cstamas
Copy link
Member Author

cstamas commented Aug 2, 2024

Am removing "swap" bit, and will make this pr same as #533

@cstamas cstamas changed the title [MRESOLVER-584] JDK transport GOAWAY fix [MRESOLVER-584] JDK transport HTTP/2 GOAWAY improvement Aug 2, 2024
Changes:
* drop 21 closer, it abruptly uses "shutdown now"
* in 11 make httpClient NOT stored per session, but per transport instance
* make client "swappable", with rudimentary GOAWAY detection
* simplify and refactor, put things in places like insecure mode config

---

https://issues.apache.org/jira/browse/MRESOLVER-584
fire a thread for that that will eventually die off.
@cstamas
Copy link
Member Author

cstamas commented Aug 2, 2024

@scholzi100 am about to merge this one, as I changed JIRA goal also from "fix" to "improve", which this PR does. And later we can refine things even more. This improvement is in fact very welcome simplification, and the improvement part is basically shortening the lifespan of client.

@cstamas cstamas merged commit 78d8eb6 into apache:master Aug 2, 2024
5 checks passed
@cstamas cstamas deleted the MRESOLVER-584-jdk branch August 2, 2024 13:19
@cstamas cstamas added this to the 2.0.1 milestone Aug 2, 2024
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.

None yet

1 participant