Undertow worker pool thread leak caused by ajax fallback requests #409
Conversation
Replaces never timeouting core.async channel read by promise with timeout.
|
@kajism Hi Karel, thanks for pinging about this- and for the PR. Will add some specific comments shortly. |
| @@ -1,8 +1,8 @@ | |||
| (ns taoensso.sente.server-adapters.undertow | |||
| "Sente server adapter for ring-undertow-adapter." | |||
| "Sente server adapter for ring-undertow-adapter. | |||
| Modified to avoid core.async and use promise directly and with read timeout" | |||
There was a problem hiding this comment.
Please remove the docstring change here.
The mention of core.async seems like an implementation detail, and the read timeout we can better document elsewhere (e.g. in the constructor).
| @@ -13,6 +13,8 @@ | |||
| WebSocketConnectionCallback | |||
| WebSocketProtocolHandshakeHandler])) | |||
|
|
|||
| (def ajax-response-timeout-ms (* 60 1000)) | |||
There was a problem hiding this comment.
Would suggest instead adding this as an option to get-sch-adapter
There was a problem hiding this comment.
Relatedly: please add a docstring to get-sch-adapter to document the 2x new options:
:ajax-response-timeout-ms(and mention its default):ajax-response-timeout-val(and mention its default)
get-sch-adapter should accept zero args (use default opts), or a single map arg of opts.
Ideally the PR would have 2x commits:
- 1st commit to introduce the new options, but keep the old default behaviour (non-breaking).
- 2nd commit to change the default, which we can mark as potentially breaking.
| (when on-close (on-close ch false nil)) | ||
| (reset! open?_ false) | ||
| (async/close! ch)) | ||
| (when @open?_ |
There was a problem hiding this comment.
Please use (when (compare-and-set! open?_ false) ...) here instead
| @@ -40,24 +42,28 @@ | |||
| (read! [this]) | |||
| (close! [this])) | |||
|
|
|||
| (deftype SenteUndertowAjaxChannel [ch open?_ on-close] | |||
| (deftype SenteUndertowAjaxChannel [promised-response open?_ on-close] | |||
There was a problem hiding this comment.
To match the surrounding code style, please suffix promised-response with an _ to indicate a dereffable.
| (read! [this] (async/<!! ch)) | ||
| (send! [this msg] (deliver promised-response msg)) | ||
| (read! [this] | ||
| (let [resp (deref promised-response ajax-response-timeout-ms :lp-timed-out)] |
There was a problem hiding this comment.
Two changes requested here:
- For compatibility with the old behaviour, please make the timeout optional.
- Suggest the timeout value be configurable via
get-sch-adapteroption. Maybe a default value of:undertow/ajax-response-timeout? (ajax-response-timeoutto better match the:ajax-response-timeout-msoption name, and theundertowprefix to help clarify that this is an adapter-specific keyword).
According to @ptaoussanis 's suggestions.
|
@ptaoussanis Hello Peter, I have commited the backward compatible suggested changes and will verify it in production. |
|
@kajism Thanks! Content of your PR has been merged on 2022-09-20-pr-409 branch and pushed to I don't use Undertow, so it'd be helpful to get your confirmation that this version:
Once I've got your confirmation, I'll merge with master. Cheers |
|
@ptaoussanis Peter, there seems to be a problem on line 44. I don't see a 3 arity version of clojure.core/deliver ... |
|
@ptaoussanis We have moved to Undertow from http-kit after experiencing huge memory leaks in allocated direct memory buffers. After a few days direct memory went up to the max heap size (4GB) and then some short videos on our page stopped playing. The problem is that direct memory is not allocated on the heap, so there was 4GB of heap + 4GB in those buffers. At first, it was hard to find the the problem, because almost everybody talks only about the heap when speaking about JVM memory. But in the system the number of memory was almost 2 times higher. Also jconsole don't show this memory. It can be found using JMX beans (java.nio:name=direct,type=BufferPool MemoryUsed). We are sending quite huge ws messages (sometimes more than 1MB), so I was suspecting this may be the reason. After move to Undertow this problem disappeared. In our case, Undertow allocates only up to 90MB in direct memory buffers. But then we experienced the worker thread starvation... I have investigated this further and created this http-kit issue: http-kit/http-kit#496 |
@kajism Apologies, fixed. And updated SNAPSHOT. |
Thanks. Locally works fine. Will be deployed in a few days and will confirm then. |
|
Works fine. Thanks! |
|
Great, appreciate the confirmation 👍 Cheers |
We were experiencing Undertow worker pool starvation in production. It was caused by ajax requests waiting on never timeouting core.async/<!! channel read. It works better after replacing core.async channel by promise with deref timeout.