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

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

Open
RokLenarcic opened this issue Jun 2, 2024 · 4 comments
Open

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

RokLenarcic opened this issue Jun 2, 2024 · 4 comments

Comments

@RokLenarcic
Copy link

RokLenarcic commented Jun 2, 2024

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
Copy link
Author

Screen.Recording.2024-06-02.at.18.58.41.mov

@ptaoussanis
Copy link
Member

ptaoussanis commented Jun 3, 2024

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
Copy link
Author

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
Copy link
Member

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 👍

@ptaoussanis ptaoussanis changed the title Listener doesn't get called sporadically Block on handler being ready when creating new Pub/Sub listener? Jun 3, 2024
@ptaoussanis ptaoussanis changed the title Block on handler being ready when creating new Pub/Sub listener? Block on handler being ready when creating new listeners? Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants