search

sunng87 / ring-jetty9-adapter

An enhanced version of jetty adapter for ring, with additional features like websockets, http/2 and http/3
Eclipse Public License 1.0
267 stars 48 forks source link

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

Closed jimpil closed 1 year ago

jimpil commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

jimpil commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

jimpil commented 1 year ago

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

lispyclouds commented 1 year ago

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

lispyclouds commented 1 year ago

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

jimpil commented 1 year ago

This can also be closed πŸ‘

darkleaf commented 1 year ago

Could you please provide the full example?

lispyclouds commented 1 year ago

This is how I'm using it.

darkleaf commented 1 year ago

It leads to physical threads leak

Π‘Π½ΠΈΠΌΠΎΠΊ экрана 2023-05-07 Π² 00 28 55

Usually my VPS runs 350 threads.

lispyclouds commented 1 year ago

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 commented 1 year ago 
  (: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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@darkleaf Java has had an excellent http-client since version 9 πŸ˜„

darkleaf commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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