Preserve thread bindings when running fetchers

[nicoberger-a]

When urania runs a fetcher, either from run-fetch or run-fetch-multi, it submits a function to an executor so it runs on a different thread.

However, it should wrap the function with bound-fn to preserve any bindings that might exist on the current thread. Not doing so could result in unexpected results if the function relies on dynamic values shared via thread bindings, like a DB connection or logging configuration.

Relevant code:

urania/src/urania/core.cljc

Lines 197 to 206 in 4043efe

	(defn- run-fetch
	[{:keys [executor env]} muse]
	(prom/create
	(fn [resolve reject]
	(-execute executor #(-> (try (-fetch muse env)
	;; fast fail if datasource throws accidentally
	(catch #?(:clj Exception :cljs :default) e
	(prom/rejected e)))
	(prom/then resolve)
	(prom/catch reject))))))

[oliyh]

Hi,

Thanks for the contribution. I have had similar issues with logging contexts too.

You mention a db connection - for this you should be using the :env option. This made me wonder whether we should also explicitly pass in :bindings as an argument. The reason I mention this is because there is plenty of promesa chaining going on within Urania and, at least with older versions of promesa, different steps of a chain could be run on separate threads. This makes me think that this relies on an implementation detail (that run-fetch is called in your code's thread) which may either break in future or limit implementation possibilities.

What are your thoughts?

[nicoberger-a]

Hi @oliyh, thanks for looking into my proposed changes, and thanks for the mention of :env. I wasn't aware of that option, and I'll keep it in mind.

I'm not totally sure what would be the implications of using :bindings vs current thread bindings. I'd probably be fine using either approach. Sorry I can't help much here.

[oliyh]

Hi @nicoberger-a

You can achieve what you need using :env, here's an example:

(def ^:dynamic *dynamic-value* 1)

#?(:clj
   (deftype Dynamic []
     u/DataSource
     (-identity [_] :some-id)
     (-fetch [_ env]
       (with-bindings (:bindings env {})
         (prom/resolved *dynamic-value*)))))

#?(:clj
   (deftest thread-bindings-test
     (assert-ast [1] (u/collect [(Dynamic.)]))

     (assert-ast [2] (u/collect [(Dynamic.)]) identity {:env {:bindings {#'*dynamic-value* 2}}})))

How does that seem to you?

Cheers

[nicoberger-a]

Hi @oliyh,

Thanks for the example. It helped me understand env and :bindings better.

That would work, in the sense that any potential bindings could be passed in that way, and the function could do anything with those values.

However, would adding bound-fn preclude somebody from using :bindings instead? I think it just gives more options, as somebody could choose to either use the calling thread bindings, or pass them through :bindings.

For reference from other projects, lacinia uses bound-fn here, even though there's also a context involved that would allow to pass bindings through. As you might know, lacinia also makes good use of promises and async code.

To explain more about my use case, I'm using lacinia with superlifter (thanks by the way, it's amazing!), and I use timbre for logging. Timbre is configured via a ^:dynamic var. The code that configures logging is far from the code that interacts with urania/superlifter/lacinia. It would be nice if I could just log/info from anywhere, without having to carefully "reestablish" the logging configuration first. That's what adding bound-fn would accomplish.

To summarize, I understand that more complex scenarios could need something different than bound-fn for "binding conveyance". Fortunately, urania allows passing an environment that can help in those cases. For more simple scenarios, adding bound-fn could make things easier.

Thanks again, and I look forward to your thoughts.