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

Thread used to evaluate forms changes over time #16

Closed
cemerick opened this Issue Nov 6, 2017 · 13 comments

Comments

Projects
None yet
5 participants
@cemerick
Copy link
Collaborator

cemerick commented Nov 6, 2017

See this thread for reference: https://groups.google.com/forum/#!topic/clojure/UfTQvYNf1-s

When a session is left idle for a little bit of time, the thread id evaluating forms changes. This causes any thread local variables in use to reset to their initial values. This causes strange behavior when doing interactive programming that involves use of thread locals.

The solution is for the same thread to always be used for a given session for as long as the session is open.

(from https://dev.clojure.org/jira/browse/NREPL-80)

@cemerick cemerick added the Bug label Nov 6, 2017

@blueberry

This comment has been minimized.

Copy link
Contributor

blueberry commented Jul 22, 2018

I was bitten by this countless times. I thought this was intentionally designed like that! :)

It is not that theread locals are reset, though. They are still in their right place, but since the thread in which the next form is being evaluated changed, the thread locals in that new thread are different, and unset it they have never been set in that new thread.

I came to this thread by looking if there is some API into nREPL's thread pool that would at least enable me to somehow control it from the repl, or at least to automate particular thread locals setting.

I might try to fix this or provide some workaround, but since I am a total nREPL noob, I would need some directions...

@arrdem

This comment has been minimized.

Copy link
Contributor

arrdem commented Jul 23, 2018

Hummmmm pinning per thread definitely makes sense - thread per connection is a well understood pattern - but couldn't you also just read and store the entire thread binding frame at the end of each eval?

@blueberry

This comment has been minimized.

Copy link
Contributor

blueberry commented Jul 23, 2018

I am not sure what you are referring to. Do you suggest to do this automatically as a part of nrepl machinery, or manually as a user? If the later, that is exactly the problem: when using 3rd party libraries that maintain their own per-thread context (CUDA in my case) REPL needs constant attention because I never know when the underlying thread is going to change and need to re-setup the context manually each time just in case.

@arrdem

This comment has been minimized.

Copy link
Contributor

arrdem commented Jul 23, 2018

Gotcha. I was referring specifically to the thread local binding state Clojure maintains, not to other JVM thread local state stores. For those yeah single-thread consistency is probably the best fix.

@bbatsov

This comment has been minimized.

Copy link
Contributor

bbatsov commented Jul 23, 2018

It seems that really that has to do with the use of futures in nREPL (e.g. https://github.com/nrepl/nREPL/blob/master/src/clojure/nrepl/server.clj#L26), as there's no "manual" thread management anywhere - just futures and delays if memory serves.

That's definitely one of the nastier bugs that we need to address.

@blueberry

This comment has been minimized.

Copy link
Contributor

blueberry commented Jul 23, 2018

@bbatsov And the configuration seems to be exactly here: https://github.com/nrepl/nREPL/blob/aea4aeab51e8444514cb3d1a5c1d388d866102f7/src/clojure/nrepl/middleware/interruptible_eval.clj#L152

What is seen there is exactly how it behaves: an unbound thread pool with a 30 seconds keep alive time. We could change these with other more convenient values, but I am not sure whether that would have some unintended consequences to the actual futures in the user's application (how are nREPL JVM related to the JVM the user's code run in? I guess it is the same JVM instance.)

Maybe there is an even better and less risky quick fix: ThreadPoolExecutor's first argument is corePoolSize, and this is currently set to 0 for all Java 6+ setups. If we set this to always be 1, there should always be at least 1 thread available (in the case a future is not already running, that is...).

If REPL has its own pool that is separate from the Clojure's default pool, that should solve the problem for most (if not all?) cases, but if those are mixed, we need something better...

I am not sure how to try this with my copy of Cider+Prelude (and am even not sure should I, since that might break my local copy of Cider, which I still do not know how to repair :)

@bbatsov

This comment has been minimized.

Copy link
Contributor

bbatsov commented Jul 23, 2018

What is seen there is exactly how it behaves: Unbound thread pool with a 30 seconds keep alive time. We could change these with other more convenient values, but I am not sure whether that would have some unintended consequences to the actual futures in the user's application (how are nREPL JVM related to the JVM the user's code run in? I guess it is the same JVM instance.)

I stand corrected. :-) At least now it seems we have a very a good idea how to fix this. ;-) It's the same JVM instance.

I am not sure how to try this with my copy of Cider+Prelude (and am even not sure should I, since that might break my local copy of Cider, which I still do not know how to repair :)

You just have to build a local copy of nREPL and test with it (using say cider-connect). Some barebone instructions are available here http://nrepl.readthedocs.io/en/latest/hacking_on_nrepl/ Afterwards you can start the server with https://github.com/nrepl/lein-nrepl, as unfortunately lein repl and boot repl still use the legacy tools.nrepl.

@bbatsov

This comment has been minimized.

Copy link
Contributor

bbatsov commented Jul 23, 2018

 ; ThreadPoolExecutor in JDK5 *will not run* submitted jobs if the core pool size is zero and
    ; the queue has not yet rejected a job (see http://kirkwylie.blogspot.com/2008/10/java5-vs-java6-threadpoolexecutor.html)
    (ThreadPoolExecutor. (if jdk6? 0 1) Integer/MAX_VALUE
                         (long 30000) TimeUnit/MILLISECONDS
                         ^BlockingQueue queue
                         thread-factory)))

This code was written when the only JDKs were 5 and 6 and it's a classic example of how not to check for an upper/lower boundary. Guess the intent to was to have the same behaviour of 5 and 6, but now we support only 8+, so we don't care about them.

It's fine to have many thread pools. I didn't write this code myself, so I'll have to get educated on it as well.

blueberry added a commit to blueberry/nREPL that referenced this issue Jul 23, 2018

blueberry added a commit to blueberry/nREPL that referenced this issue Jul 23, 2018

@blueberry

This comment has been minimized.

Copy link
Contributor

blueberry commented Jul 23, 2018

@bbatsov So far, this fix works well for me with no special setup in my projects other than starting it with lein nrepl and using M-x cider-connect.

@blueberry blueberry referenced this issue Jul 23, 2018

Merged

Fix #16. #36

@bbatsov bbatsov closed this in 14bcf7f Jul 24, 2018

@bbatsov bbatsov reopened this Jul 26, 2018

@bbatsov

This comment has been minimized.

Copy link
Contributor

bbatsov commented Jul 26, 2018

As discussed with @vspinu I'm reopening this, as #36 solved the problem only partially. Ultimately we'll need to come up with more extensive changes that ensure sessions are mapped to a single thread that never changes.

@cgrand

This comment has been minimized.

Copy link
Contributor

cgrand commented Dec 4, 2018

If you map sessions to threads then it simplifies code a lot (look at all the references to get-thread-bindings for a start). However you lose interruptible eval.

Another possibility is to use reflection so as to save thread locals (and inheritable thread locals for exhaustiveness) like we do with thread bindings (actually it would save thread bindings too since they are stored in a ThreadLocal). The catch? Reflection.

@bbatsov

This comment has been minimized.

Copy link
Contributor

bbatsov commented Dec 4, 2018

However you lose interruptible eval.

Yeah, that's a great point. You should also take a look at the subsequent conversation we've had here #62 on the subject.

The catch? Reflection.

How big of an impact do you think this is going to to have?

@cgrand

This comment has been minimized.

Copy link
Contributor

cgrand commented Dec 10, 2018

Having thought a bit more about it, I have a new proposal:
• allocate a dedicated thread when a session is created by clone,
• document that interrupting an eval will cause the thread to be cycled for another one.

cgrand added a commit that referenced this issue Dec 11, 2018

@cgrand cgrand referenced this issue Dec 12, 2018

Merged

Session thread #98

@bbatsov bbatsov closed this in adb1c14 Dec 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.