search

eclipse-ee4j / jersey

Eclipse Jersey Project - Read our Wiki:
https://github.com/eclipse-ee4j/jersey/wiki
Other
688 stars 351 forks source link

Project Loom/JDK 21 compatible #5545

Closed patrickjamesbarry closed 2 weeks ago

patrickjamesbarry commented 6 months ago

We are preparing ourselves to upgrade to jdk 21, and was excited to see the addition of the JavaNetHttpConnectorProvider, because it would result in less dependencies to manage and automatically support virtual threads. However, after scanning the code in this repo, I see usages of synchronized methods which will result in pinned threads. Is there work under way to replace these locks with locking strategies that will not result in pinning the thread? If so, what version can I look forward to trying out that is more "project loom" friendly?

jansupol commented 6 months ago

That's a good point, we planned to revisit synchronized blocks all over the code.

patrickjamesbarry commented 6 months ago

Will the use of ThreadLocals migrate to using some of the new scoping functionality in 21? ie ScopedValue and StructuredTaskScope? Will this lead to just a change in behaviour for Jersey's own @RequestScope?

jansupol commented 6 months ago

ThreadLocals might or might not be an issue, depending on what the ThreadLocal holds. If the hold value is a subject just for a single virtual thread, it is not that an issue, afaik.

But yes, the ThreadLocal use should be revisited, too.

jansupol commented 5 months ago

We have replaced the synchronized blocks for 3.1.6, along with minor ThreadLocal changes to clear the storage. However, right now we do not plan to drop the ThreadLocals as they work with virtual threads. We revisited the usage and it should not cause any harm with virtual threads. The Jersey @RequestScope behavior should not be affected. We do not plan to use ScopedValues (still maintaining support for JDK 11) unless there would be a reason. Do you have any actual bottleneck in mind?

That said, Jersey works quite well with Loom in Helidon 4 (JDK 21) already.

wendigo commented 5 months ago

@jansupol when is the 3.1.6 release happening? We (https://github.com/trinodb/trino) would love to try it out with virtual threads :)

jansupol commented 5 months ago

@wendigo The next week, likely.

wendigo commented 5 months ago

Great! Thanks @jansupol

celtric commented 4 months ago

What would be the appropriate way to enable virtual threads when using jersey-container-jetty-http or jersey-container-jetty-http2?

We're currently using JettyHttpContainerFactory, but the thread pool is apparently hardcoded https://github.com/eclipse-ee4j/jersey/blob/3.1/containers/jetty-http/src/main/java17/org/glassfish/jersey/jetty/JettyHttpContainerFactory.java#L257

jansupol commented 3 months ago

We have the CommonProperties#THREAD_FACTORY and CommonProperties#USE_VIRTUAL_THREADS properties now. Does it suffice your needs @celtric ?

mkarg commented 2 weeks ago

I wonder why this issue is still open, as apparently the intended work is done in Jersey 3.1.6+

celtric commented 2 weeks ago

We have the CommonProperties#THREAD_FACTORY and CommonProperties#USE_VIRTUAL_THREADS properties now. Does it suffice your needs @celtric ?

Sorry for the late reply, missed your comment. I confirm CommonProperties#USE_VIRTUAL_THREADS works, thank you!

senivam commented 2 weeks ago

Closing as resolved