Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix implementation of heartbeat scheduling #250

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
Member

janherich commented Feb 24, 2014

Heartbeat functionality in pedestal uses class Executors
from package java.util.concurrent, its method newScheduledThreadPool
is used to initialize thread pool for sse heartbeat functionality.
However, threads created by default are non-daemon threads where
they really should be rather daemon threads, so they don't prevent
the JVM process from exiting.

My fix supplies ThreadFactory for newScheduledThreadPool, this factory
just sets every jvm thread in the thread pool to be daemon thread.

janherich added some commits Feb 24, 2014

@janherich janherich Fix implementation of heartbeat scheduling
Heartbeat functionality in pedestal uses class Executors
from package java.util.concurrent, its method newScheduledThreadPool
is used to initialize thread pool for sse heartbeat functionality.
However, threads created by default are non-daemon threads where
they really should be rather daemon threads, so they don't prevent
the JVM process from exiting.

My fix supplies ThreadFactory for newScheduledThreadPool, this factory
just sets every jvm thread in the thread pool to be daemon thread.
36dfc05
@janherich janherich Formatting changes 9aa7543
Member

rkneufeld commented Mar 1, 2014

This seems reasonable to me. What do you think @ohpauleez?

Owner

ohpauleez commented Mar 2, 2014

I'd like to think about this a little more. @janherich can you describe the problem statement a little more. It starts with: "The heartbeat threads prevent my JVM from properly shutting down when _" or "When I am doing ____, the heartbeat threads ________, which stops me from being able to _______"

Can you link to any additional docs about potential other implications/tradeoffs to making them daemon threads?
Would something like Clojure's shutdown-agents function also work for your current use case?

Member

janherich commented Mar 2, 2014

Hello @ohpauleez , sure, i can provide a better description:

I discovered the problem when i was profiling my pedestal servlet application (version 0.2.2) running in the tomcat servlet container (7.0.51) with the help of VisualVM (it's default JDK profiling and monitoring tool, you can start it with JAVA_HOME/bin/jvisualvm).

After shutting down the tomcat (via default bin/shutdown.sh script), i saw in VisualVM that the tomcat instance is still running, at first i was suspecting that the threadpool for core.async go "threads" may be the culprit, but then i discovered that all core.async threads are daemon threads and i saw that it was indeed the threadpool (or threads created in this threadpool) responsible for pedestal heartbeat functionality.

After i installed patched version in my local maven repository and run the same scenario again, there were no such problems and the servlet container exited correctly on shutdown.

The basic difference between normal and daemon threads on JVM is that daemon threads don't prevent JVM from exiting (specifically, JVM exits when there are no more non-daemon normal thread running).
You don't want to have important logic run in daemon threads, because when JVM is stopped, daemon threads are stopped immediately, so for example file copy task run in daemon thread is a bad idea.

On the opposite, daemon threads are ideal for non-crucial, supporting tasks which are relevant only when the main application (on which behalf the daemon thread is acting) is running.
I believe that SSE heartbeat functionality is a typical example of such supporting task.

Regarding your last question, yes, some hook as shutdown-agents would solve the situation as well, but i think it's worse (needlessly manual) solution to this problem.

http://docs.oracle.com/javase/7/docs/api/java/lang/Thread.html
http://stackoverflow.com/questions/2213340/what-is-daemon-thread-in-java

Member

janherich commented Mar 8, 2014

Any progress on this ?

Owner

ohpauleez commented Mar 10, 2014

It's slotted for release in 0.3.0. There are still a few things we're finishing up for the release. Thanks for the comment above, it was exactly what I was looking for.

Member

janherich commented Mar 10, 2014

Thanks for the information, i'm really curious about 0.3.0 release and the interceptor engine based on core.async channels.

I have many suggestions and requests for improvement (mainly related to better Servlet 3.0-3.1 support, including support for asynchronous listeners - better SSE, better non-blocking file-uploads, etc.), but i will wait till 0.3.0 is released.

Owner

ohpauleez commented Mar 10, 2014

Well, it's easier to take consideration now while in the middle of all the work. Write a Gist of your ideas and any additional links to sources/references to fill in context. Do your best to describe problem scenarios instead of solutions/features you'd like to see in Pedestal - that helps us evaluate overall design decisions. Link to the Gist in a comment here.

Member

janherich commented Mar 12, 2014

Sorry, i was pretty busy last days, but finally i managed to to write down my ideas #253

Owner

ohpauleez commented Apr 18, 2014

This is now on master - thanks!

@ohpauleez ohpauleez closed this Apr 18, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment