Dispatcher.compose doesn't return a true dispatcher

[ronag]

It doesn't:

• forward dispatcher events
• forward .dispatch return value
• handle async dispatches in terms of backpressure

Fixing these are quite complicated.

I suggest DIspatcher.compose is replaced by undici.compose which just returns a dispatch function and then we add support for all places that take a dispatcher argument should also check if it is a function and use it accordingly.

Refs: #3368

[ronag]

@metcoder95 FYI

[metcoder95]

Yeah, that was my biggest concern when trying to mimic the Dispatcher being returned.

But just to understand clear, you suggest to instead of doing mimics of the dispatcher we just do that for the dispatch; so it can be injected instead of the dispatcher?

[ronag]

Yea, something like that. The back-pressure mechanism if dispatcher is slightly broken.

[ronag]

Dispatcher.dispatch always receives the tasks and then return true if it can receive further requets, otherwise false. If false is returned then it will emit a drain event.

This gets more complicated with Agent as the back-pressure is per opts.origin and hence the drain needs to check for the corresponding origin in the drain event (not sure if this is actually working atm, need to check).

So far everything could work (although complicated).

The tricky part occurs when we have interceptors (e.g. retry, redirect) that perform sub dispatches. There are two ways this occurs (1) sub dispatches in the Handler or (2) async logic in the interceptor dispatch. Not sure how to handle this or whether or not it's actually a problem.

[metcoder95]

Dispatcher.dispatch always receives the tasks and then return true if it can receive further requets, otherwise false. If false is returned then it will emit a drain event.

Hmm, good point; this is something that is definitely missed from the current interceptor implementation.

The tricky part occurs when we have interceptors (e.g. retry, redirect) that perform sub dispatches. There are two ways this occurs (1) sub dispatches in the Handler or (2) async logic in the interceptor dispatch. Not sure how to handle this or whether or not it's actually a problem.

I might say that the biggest issue is that we are branching the chain of calls there. In theory, we should try to always reply with true|false to allow backpressure do its things, but in truth we do it blindly without fully respecting that mechanism.
Specially on retry that we just fork the chain and fail if we exhaust the retries, I do not think that under high-pressure we will behave well.

Maybe the best is to always mandate return the value of dispatch so it can be called on any time, but I might need to draft a PoC to see if this is something feasible (I'm currently blindly saying it is).

[ronag]

Another option to consider is tryDispatch which will not take the request if the dispatcher is busy.

Dispatcher.tryDispatch(opts, handler, () => /* try again */)

and then we can deprecate the return value if dispatch and the drain event.

Possibly even deprecating all events from the Dispatcher API.

[ronag]

Or just add another param to dispatch that changes the behavior from fire and forget to fire and retry, Dispatcher.dispatch(opts, handler, onDrain)

[ronag]

A lot of work remaining: #3374

[metcoder95]

Another option to consider is tryDispatch which will not take the request if the dispatcher is busy.

Dispatcher.tryDispatch(opts, handler, () => /* try again */)

and then we can deprecate the return value if dispatch and the drain event.

Possibly even deprecating all events from the Dispatcher API.

Do we want to deprecate them?
I provide my feedback based on the fact that we will preserve them, but if we want to tear them down then it is not valid anymore.
What will be the value of deprecating for this new API instead of the events?