re-frame icon indicating copy to clipboard operation
re-frame copied to clipboard

[Bug]: Non-daemon executor thread in Clojure prevents clean JVM shutdown

Open oliyh opened this issue 1 year ago • 9 comments

What happened?

Hi,

Firstly thank you for the great library, it transformed the way I thought about building UIs and it's so much more elegant than redux and so much cleaner than React hooks.

I have a library called re-graph which uses re-frame mostly for state management of websocket lifecycle. It runs in both cljs and clj, and we have uncovered an issue with the clj implementation which is preventing clean shutdown of the JVM because of an executor thread which is left running.

The full conversation is here: https://github.com/oliyh/re-graph/issues/75

@lowecg has produced some code which works around the issue, but I think this would be best incorporated into re-frame (or maybe re-frame-test) and I'd be grateful to know what you think.

Thanks for your time

Version

1.2.0

What runtimes are you seeing the problem on?

JVM (CLJC)

Relevant console output

No response

oliyh avatar Nov 15 '24 09:11 oliyh

Hey, I'd be happy to look into this. However, day8 are in crunch mode, so we can't spare much time until next Spring.

Please make a PR with explanation & references to the java features. The more straightforward the writing, the sooner we can merge it.

kimo-k avatar Nov 15 '24 20:11 kimo-k

I've created a minimal project demonstrating the issue: https://github.com/lowecg/re-frame-issue-809

I'll issue a PR shortly

lowecg avatar Nov 17 '24 15:11 lowecg

@kimo-k / @oliyh Thanks for you time on this. PR raised - I hope it meets expectation :-) Let me know if you need anything else.

lowecg avatar Nov 17 '24 16:11 lowecg

My takeaways so far:

kimo-k avatar Dec 02 '24 12:12 kimo-k

Thanks @kimo-k, that's accurate.

some people want to https://github.com/oliyh/re-graph/issues/75#issuecomment-2478108942. What exactly does that mean?

If you use AWS Lambda with the JVM, then you want to likely want to use SnapStart to cut down cold start times. SnapStart requires you to prime your app by touching necessary classes in a warm up phase. This hits the executor.

I've just been doing some testing without interfering with the executor and it's working ok. So let's leave out the reuse after shutdown

lowecg avatar Dec 02 '24 13:12 lowecg

The PR where we added the executor has the premise that building a full app in .clj is a non-goal for re-frame.

I suppose that premise still holds - we have no ambitions to fully support .clj apps.

That seems to rule out the need to change re-frame so that re-graph can work. Maybe it's better if re-graph can just use the workaround.

However if this problem affects re-frame-test, that does seem more serious.

kimo-k avatar Dec 02 '24 13:12 kimo-k

@lowecg, @oliyh, in your opinion, would it suffice to remove the executor altogether? Instead, next-tick would simply be (fn next-tick [f] (f) nil). We'd no longer call event handlers on a separate thread.

Looking at the context so far, it seems like the benefits of the executor could be more cosmetic than pragmatic. And we have observed at least one drawback to the executor (#469, #471).

kimo-k avatar Dec 02 '24 13:12 kimo-k

Hi @kimo-k thanks for the time you have spent on this. I'm agnostic about how next-tick is implemented, I consider that re-frame internals and I don't know of anything that depends on this.

oliyh avatar Dec 02 '24 13:12 oliyh

@kimo-k - I agree with @oliyh. I'm happy to use the workaround in the meantime.

lowecg avatar Dec 02 '24 16:12 lowecg