taoensso / carmine

Redis client + message queue for Clojure
https://www.taoensso.com/carmine
Eclipse Public License 1.0
1.16k stars 131 forks source link

Block on handler being ready when creating new listeners? #310

Open RokLenarcic opened 3 months ago

RokLenarcic commented 3 months ago

I have this test namespace:

(ns memento.redis.listener2-test
  (:require [clojure.test :refer :all]
            [taoensso.carmine :as car]))

(defn listener [conn f]
  (car/with-new-pubsub-listener conn
    {"test-chan" (fn [[typ _ msg]] (when (= "message" typ) (f msg)))}
    (car/subscribe "test-chan")))

(deftest listener-subscription-test
  (testing "messages work"
    (let [msgs (atom [])]
      (with-open [l (listener {} #(swap! msgs conj %))]
        (car/wcar {}
          (car/publish "test-chan" [1]))
        (while (empty? @msgs)
          (Thread/sleep 100))
        (is (= [[1]] @msgs))))))

Around 5% of the time running this test (in the same REPL, so not a new JVM for every run) it hangs (on the while statement). If I change while statement to just a sleep call, the test fails around 5% of the time instead of hanging. I was unable to figure out why that is. It is like sometimes the pub/sub system fails to deliver the message.

RokLenarcic commented 3 months ago

https://github.com/taoensso/carmine/assets/3322433/45ed9ba0-2418-456a-ac09-410eac5f4a2a

ptaoussanis commented 3 months ago

Hi Rok, I don't have an opportunity to investigate this in detail right now - but my first inclination would be try adding a sleep between creating your listener, and executing your first publish call/s.

Does that help?

Motivation for checking this: the current sequence looks like it might be fragile since you're creating a listener (which needs to spin up a handler thread), then you're immediately publishing. If the handler thread isn't active by the time Redis pushes the published message, it'll be dropped.

I'll check back on this later. If my hunch is correct, and listener call indeed returns before the handler thread is guaranteed to be active - this'd definitely be nice to improve (listener call could block on active handler thread before returning). PR welcome, otherwise I'll try make the change myself later this week.

RokLenarcic commented 3 months ago

It seems to help to put a sleep there. Yeah in real scenario this isn't much of a problem as these messages won't arrive immediately, but for tests it's super annoying.

ptaoussanis commented 3 months ago

Yeah in real scenario this isn't much of a problem as these messages won't arrive immediately, but for tests it's super annoying.

PR welcome if you felt like submitting one (it should be pretty straightforward), otherwise I'll look at this next time I'm on batched Carmine work.

In the meantime, would just recommend a short sleep after starting listeners to ensure that relevant thread/s are started 👍