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

Project Loom is here and Jetty 10+ already supports it #82

Closed
jimpil opened this issue Nov 14, 2022 · 28 comments
Closed

Project Loom is here and Jetty 10+ already supports it #82

jimpil opened this issue Nov 14, 2022 · 28 comments

Comments

@jimpil
Copy link
Contributor

jimpil commented Nov 14, 2022

As far as I can tell there are two viable options here:

  1. Rely on jetty's native support for virtual-threads - see this
  2. Use the existing support for a custom org.eclipse.jetty.util.thread.ThreadPool impl (via the :thread-pool option)

Unfortunately, none of the two options is free - so to speak...With the first option, someone has to figure out how to configure this (I can definitely help out), and you (as the author of this lib) would have to expose it. With the second option, things are kind of looser...

You could certainly say that there is not much to do here because option 2 is already supported...But think about this - everyone who wants to use virtual threads via option 2, has to write the same mundane Java class (which basically wraps this new executor). So I would say this actually belongs in the library.

Let me know of your thoughts...

@sunng87
Copy link
Owner

sunng87 commented Nov 16, 2022

From my perspective, I would pick approach 1 and provide an option to enable virtual-threads support explicitly. I'm not sure if there is any prerequisite for the clojure app, if it is to run on virtual-threads.

Doors are open if there is scenario to customize something, these pro users can always provide their own :thread-pool.

@jimpil
Copy link
Contributor Author

jimpil commented Nov 17, 2022

For what it's worth, here is how such a pool could look like in pure clojure:

(defn- jetty-vpool
  ([]
   (jetty-vpool "jetty-"))
  ([^String thread-prefix]
   (let [exec (-> (Thread/ofVirtual)
                  (.name thread-prefix 0)
                  .factory 
                  Executors/newThreadPerTaskExecutor)] 
     (reify ThreadPool
       (execute [_ runnable] ;; pass-through
         (.execute exec runnable))
       (join [_]
         (try ;; block indefinitely
           (.awaitTermination exec Long/MAX_VALUE TimeUnit/DAYS) 
           (catch InterruptedException _ (.shutdown exec))))
       (getThreads [_]
         (->> (Thread/getAllStackTraces) 
              .keySet
              (filter (fn [^Thread t]
                        (and (.isVirtual t)
                             (-> (.getName t)
                                 (.startsWith thread-prefix)))))
              count))
       (isLowOnThreads [_] false) ;; seems reasonable since the executor is unbounded
       (getIdleThreads [_] Integer/MAX_VALUE)))))

@sunng87
Copy link
Owner

sunng87 commented Nov 24, 2022

Thank you for providing the example. The pool implementation contains a few customization. I suggest to leave this to advanced user and use-cases.

If we go approach 1 to use jetty's native support of virtual threads, are you interested in adding support for this new option? I believe its sufficient during the preview period of virtual threads feature.

@jimpil
Copy link
Contributor Author

jimpil commented Nov 24, 2022

Ok yeah, I'll try to have a look at how to configure jetty...

@jimpil
Copy link
Contributor Author

jimpil commented Nov 25, 2022

So I wasn't able to find any concrete info on how to to configure jetty for virtual-threads, but I did find the following class:

I'm not 100% sure I'm reading this correctly, but it seems to happen automatically (given that the runtime supports it). I find it hard to believe that they would do something so 'aggressive' without an opt-in mechanism, but without anything more explicit, that's what I'm inferring.

@jimpil
Copy link
Contributor Author

jimpil commented Nov 25, 2022

I just tested JDK-19 with --enable-preview on, and by default the threads are NOT virtual (which means my earlier interpretation was wrong). So there must be further config required...

@jimpil
Copy link
Contributor Author

jimpil commented Nov 25, 2022

Ok so I tracked down the original commit that shows more or less the complete picture:

Looking at line 62 of VirtualThreadsTest.java it appears that this happens at the Connector level. I'll try to see if I can make this work via the :configurator.

@jimpil
Copy link
Contributor Author

jimpil commented Nov 29, 2022

Some good news here...It seems that the usual QueuedThreadPool implements VirtualThreads.Configurable, and so one can call .setUseVirtualThreads(true) on it. Here are the relevant links:

If that's right, all you have to do is to expose a :use-virtual-threads? option, and call (.setUseVirtualThreads ...) here. Obviously, this option will be superceded by :thread-pool, and also it's not clear what min/max threads mean at that point. I know for a fact that pooling/reusing virtual-threads is an anti-pattern, so I'm not sure what to make of this approach...

@sunng87
Copy link
Owner

sunng87 commented Dec 1, 2022

Sorry for late response. I haven't checked Jetty's QueuedThreadPool internals for how to deal with max/min threads when virtual threads enabled. But I think we can track their development by adding :use-virtual-threads? first. I guess for now it's just a marker to ask underlying libraries to use new non-blocking I/O for virtual threads.

@lispyclouds
Copy link

lispyclouds commented Mar 3, 2023

Just leaving it out here in case it's of use to anyone. I managed to get it running using Jetty 11.0.14 on JDK 19.0.2 by passing this to :thread-pool:

(doto (org.eclipse.jetty.util.thread.ExecutorThreadPool.)
  (.setVirtualThreadsExecutor (java.util.concurrent.Executors/newVirtualThreadPerTaskExecutor)))

the implementation of the ThreadPool interface is not needed and this uses the Jetty's inbuilt machinery. Should work for QueuedThreadPool in a similar way as well.

@jimpil
Copy link
Contributor Author

jimpil commented Mar 4, 2023

Nice one @lispyclouds - i can confirm it works well with QueuedThreadPool, albeit it's not clear to me what would be a good value for the maxThreads parameter so I've left it as the default (200).

@lispyclouds
Copy link

I think max and min threads are overridden if the thread pool is set?

@jimpil
Copy link
Contributor Author

jimpil commented Mar 4, 2023

They are ignored here (on this library), but what about Jetty itself? For example, with (doto (QueuedThreadPool.) (.setVirtualThreadsExecutor ...)), you get maxThreads = 200.

@lispyclouds
Copy link

Ah right, will try digging around more! Didn't know it was set to 200.

@lispyclouds
Copy link

lispyclouds commented Mar 4, 2023

I suppose Integer/MAX_VALUE should be the best we can have with this setup.

@jimpil
Copy link
Contributor Author

jimpil commented Apr 5, 2023

This can also be closed 👍

@jimpil jimpil closed this as completed Apr 5, 2023
@darkleaf
Copy link

darkleaf commented May 6, 2023

Could you please provide the full example?

@lispyclouds
Copy link

This is how I'm using it.

@darkleaf
Copy link

darkleaf commented May 6, 2023

It leads to physical threads leak
Снимок экрана 2023-05-07 в 00 28 55

Usually my VPS runs 350 threads.

@lispyclouds
Copy link

How have you configured it? Can we see the code? Also checking the thread count of just the JVM and if they are virtual threads could help too.

@darkleaf
Copy link

darkleaf commented May 7, 2023

  (:require
   [ring.adapter.jetty9 :as jetty]

  (jetty/run-jetty handler {:join? false
                            :port  port
                            :thread-pool
                            (doto (org.eclipse.jetty.util.thread.ExecutorThreadPool. (Integer/MAX_VALUE))
                              (.setVirtualThreadsExecutor (java.util.concurrent.Executors/newVirtualThreadPerTaskExecutor)))}))

java 20
--enable-preview
info.sunng/ring-jetty9-adapter {:mvn/version "0.21.0"}
INFO org.eclipse.jetty.server.Server - jetty-11.0.15; built: 2023-04-11T18:37:53.775Z; git: 5bc5e562c8d05c5862505aebe5cf83a61bdbcb96; jvm 20.0.1+9

I was watching the number of threads in htop. And it kept increasing.

Also I use newrelic agent, but without it threads are leaking too.
Снимок экрана 2023-05-07 в 10 51 22

@jimpil
Copy link
Contributor Author

jimpil commented May 7, 2023

@darkleaf Your code is correct (I'm actually using QueuedThreadPoolExecutor with the default max value of 20), but htop is not the right tool, as it reports your system threads (in total). You need an up-to-date java monitoring tool, in order to monitor virtual VS OS threads. Moreover, jetty will only use virtual threads if it makes sense - i.e. for operations that are highly likely to block (like invoking your handler). Other stuff that jetty needs to do, might use normal OS threads.

@jimpil
Copy link
Contributor Author

jimpil commented May 7, 2023

FYI, the system below has created more than 100,000 virtual threads (I can see it in the logs):
Screenshot 2023-05-07 at 13 01 57

@darkleaf
Copy link

darkleaf commented May 7, 2023

I've read The Ultimate Guide to Java Virtual Threads and enabled -Djdk.tracePinnedThreads=full.
I use apache http client 5. It has synchronized method. So I can't use virtual threads now 😞

org.apache.hc.client5.http.impl.io.PoolingHttpClientConnectionManager$3.get(PoolingHttpClientConnectionManager.java:339) <== monitors:1

@jimpil
Copy link
Contributor Author

jimpil commented May 7, 2023

@darkleaf Java has had an excellent http-client since version 9 😄

@darkleaf
Copy link

darkleaf commented May 8, 2023

Maybe I'll switch to java.net.http.HttpClient.
But Clojure itself uses synchronized methods

clojure.lang.LazySeq.sval(LazySeq.java:42) <== monitors:1
clojure.lang.LazySeq.seq(LazySeq.java:51) <== monitors:1

https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/LazySeq.java#L41-L66

Also, I use org.graalvm.js/js, it also has

com.oracle.truffle.polyglot.PolyglotLanguageContext.ensureCreated(PolyglotLanguageContext.java:612) <== monitors:1

@jimpil
Copy link
Contributor Author

jimpil commented May 8, 2023

@darkleaf You're right - Clojure itself has 17 occurrences of synchronized according to this. From a quick look, these can all be converted to ReentrantLock - I guess we need to report it to JIRA. I'll try to see if I can do that tonight...

@jimpil
Copy link
Contributor Author

jimpil commented May 8, 2023

FYI - https://clojure.atlassian.net/browse/CLJ-2771
upvote please

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

No branches or pull requests

4 participants