Superlifter wont deliver (lacinia) promise if a fetch request is rejected

[lennartbuit]

Heya 👋

Say you have some datasource, that can fail for some reason:

(defrecord Fail [nonce]
  u/Datasource
  (-identity [_] nonce)
  (-fetch! [_ _] (prom/rejected (ex-info "Not good" {}))))

With a resolver that uses with-superlifter/enqueue!:

(defn failing-resolver
  [ctx args val]
  (with-superlifer ctx
    (enqueue! (Fail. (rand-int 100)))

Then with-superlifter will produce a lacinia "resolver result" promise that is never delivered. This request will now hang.

---

I think the problem here is how enqueue works. Enqueue takes your (urania) AST, and appends a then step:

superlifter/src/superlifter/core.cljc

Lines 76 to 79 in cf4ff29

	delivering-muse (u/map (fn [result]
	(prom/resolve! p result)
	result)
	muse)]

Now, this is not the place that the urania tree is executed, that happens in update-buckets!, via (urania) u/execute!:

superlifter/src/superlifter/core.cljc

Lines 44 to 47 in cf4ff29

	(-> (u/execute! (u/collect muses)
	(merge (:urania-opts new)
	(when cache
	{:cache (->urania cache)})))

Urania, however, will short circuit asts that contain failing datasources. The all step here is rejected:

https://github.com/funcool/urania/blob/3d3c61206d8a7de675af5d97ad08706ad3307a15/src/urania/core.cljc#L236

Therefore, this then step is skipped (the promise is rejected):

https://github.com/funcool/urania/blob/3d3c61206d8a7de675af5d97ad08706ad3307a15/src/urania/core.cljc#L237-L241

And therefore the superlifter amended then is never interpreted. Urania essentially short circuits and superlifter can never deliver the promise created in enqueue! because of that.

---

So; we tried something, but I'm not necessarily sure whether that is something that we should do in superlifter like this. The rejection of the datasource does bubble up to u/execute!. So in update-buckets! we can know what calls to enqueue! resulted in a rejection.

So in this tree, I've changed the queue shape to have both a muse, and the (enqueue!) promise. So that if u/execute! fails, I can know what enqueue promises it needs to reject.

Thoughts?

[oliyh]

Hello,

Superlifter should not hang if the execution is rejected so I'm in favour of this behaviour. I need to spend a bit more time reading your implementation though.

Thanks

[lennartbuit]

Thats alright, we have workarounds in place.

I'm not fully convinced of my own implementation. I altered the shape of the queue inside superlifter (from list of muses to list of maps of muses and promises) and I'm not sure whether that is breaking. So by all means, challenge my implementation ^^.

When we agree on approach, I can add a bit of coverage, touch it up and PR, thats alright with me. Just let me know :).

[lennartbuit]

So; talking to @oliyh on slack we came to the following conclusion:

Altering the shape of the queue can be breaking, but we are okay with that. We will make the queue a list of records (instead of a list of muses) so that we can make better guarantees about its shape in the future.

If you ended up extending start-trigger! and you don't regard the queue as an opaque list, we may break your custom trigger fn. If you only slice the list, or do any list operations without relying on it being a list of muses, you'll be fine.

---

I can pick this up in a few days time, I'll continue the shared tree, add some tests and such. You know, the usual.