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

[Http-Client] Use virtual thread executor by default #442

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

SentryMan
Copy link
Collaborator

When on JDK 21+, the builder will default to the VirtualThreadPerTask Executor for the underlying HttpClient.

@SentryMan SentryMan added the enhancement New feature or request label Jun 2, 2024
@rob-bygrave
Copy link
Contributor

Did someone request this change or do we just think it's a good idea?

@SentryMan
Copy link
Collaborator Author

I just think it's cool

@SentryMan SentryMan self-assigned this Jun 5, 2024
@SentryMan
Copy link
Collaborator Author

@rbygrave yea or nay?

@rbygrave
Copy link
Contributor

rbygrave commented Sep 7, 2024

yea or nay?

As I see it, this looks like a good idea but I'm not quite 100% sure myself. So I'm sitting on the fence waiting for someone to hit a use case where they want this as the default. I guess the JDK HttpClient has to be more conservative which is perhaps why this isn't the default in the JDK.

If we merge this so that it becomes the default behaviour then it is hard to "undo" that in that doing so would be a "silent regression" type of change.

In this sense we need to be 100% sure when/if we merge this. For myself I'm not quite 100% yet. Can we keep this on hold for now?

@SentryMan
Copy link
Collaborator Author

I'm hearing nay (at least for now)

@SentryMan SentryMan closed this Sep 7, 2024
@rbygrave
Copy link
Contributor

rbygrave commented Sep 7, 2024

Ok, but if we leave the PR open ... then other people can see it and add a comment. If we delete the PR it's now really unlikely to happen?

[My preference was to keep the PR open]

@SentryMan SentryMan reopened this Sep 7, 2024
@SentryMan SentryMan added the help wanted Extra attention is needed label Sep 7, 2024
@re-thc
Copy link
Contributor

re-thc commented Oct 8, 2024

Maybe best to wait until JEP 491 gets merged

@SentryMan
Copy link
Collaborator Author

Maybe best to wait until JEP 491 gets merged

I mean the current implementation of HTTP client doesn't use synchronized anyways

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants