race condition in upmap causes null pointer exception

[shaunlebron]

Using upmap sometimes results in this exception:

java.lang.NullPointerException: Cannot invoke "java.util.concurrent.Future.get()" because "fut" is null
core.clj:2317	clojure.core / deref_future
core.clj:2337	clojure.core / deref
impl.clj:58	com.climate.claypoole.impl / deref_fixing_exceptions
impl.clj:58	com.climate.claypoole.impl / deref_fixing_exceptions

It appears that pmap-core zips the ordered tasks with an unordered queue of derefable results of those tasks, and sometimes the second value (returned by a blocking take from the unordered queue) returns nil, causing deref-fixing-exceptions to deref that nil value, throwing the above exception.

So, something in the implementation of the blocking take from the unordered queue is causing it to return nil. I can only guess this is a race condition because it seemed to happen randomly. Anecdotally, I saw it happening more in 1.1.4 than in 1.2.2.

I don’t have steps to reproduce, but we fixed the issue in our system by just switching from upmap to pmap, which avoids what I think is the problematic zip operation above.

[karlmikko]

claypoole/src/clj/com/climate/claypoole.clj

Lines 251 to 256 in fb0cce3

	:else (let [;; We have to get the casts right, or the compiler will choose
	;; the (.submit ^Runnable) call, which returns nil. We don't
	;; want that!
	^ExecutorService pool* pool
	^Callable f* (impl/binding-conveyor-fn f)
	fut (.submit pool* f*)]

claypoole/src/clj/com/climate/claypoole.clj

Lines 267 to 268 in fb0cce3

	Future
	(get [_] (.get fut))

The exceptions happens when it tries to use the future created from the Executor submit. As the comment above notes, the task needs to be a Callable not a Runnable in order to return a future, else it returns null.

I wonder if the type hint is somehow being missed from the clojure compiler or JVM optimisations. Would it be worth wrapping in a reified object?

(defn- callable
  [f]
  (with-meta
    (reify
      java.util.concurrent.Callable
      (call [_]
        (f)))
    (meta f)))

f* (callable (impl/binding-conveyor-fn f))
fut (.submit pool* f*)]

---

The other thing that could be happening is that nil is put on the unordered queue. The difference between the ordered and unordered queues is, ordered is added sequentially in a doseq and the unordered is added from inside the executing thread. I wonder if 2 threads try to add to the unorded queue at the same time one of them adds nil instead. Potentially even when LinkedBlockingQueue is expanding to make room could cause more to wait and add at the same time.

claypoole/src/clj/com/climate/claypoole.clj

Lines 366 to 367 in fb0cce3

	(impl/queue-seq-add!
	unordered-results-q @p)))

Potentially locking when adding to the unordered queue may help?

(locking 
   unordered-results-q
  (impl/queue-seq-add!
    unordered-results-q @p))

The thing that makes this seem unlikely is the error having a stack trace in the reified future, as above, as if nil was in the queue we would get a different error trying to deref it.

---

I am seeing the issue very occasionally when using upmap, I have changed from upmap to pmap and that seems to have resolved the issue for me also.

I am getting the issue in amazon corretto 21, clojure 1.12.0

I am using a long lived threadpool (for the life of the app) created with threadpool NOT a priority thread pool (as I know they do some reification.