Add support for asynchronous dispatch #37
Conversation
Introduce a configurable scheduler option for NewEventDispatcher and add two asynchronous schedulers, one that is unlimited and one that uses a bounded queue and a fixed number of workers. For many applications, switching to asynchronous handling will be as easy as adding this new option when creating the event dispatcher. Other application will need to provide a custom ContextDeriver to copy values from the request context into the detached handler context. As a result of error handling code introduced as part of this change, go-githubapp now requires at least Go 1.13.
|
|
||
| The following schedulers are included in the library: | ||
|
|
||
| - `DefaultScheduler` - a synchronous scheduler that runs event handlers in |
There was a problem hiding this comment.
is it worth naming this something that's more descriptive of the function? InlineScheduler or BlockingScheduler dont feel right... i can't come up with a better name, maybe default works.
the question applies to most things named default below, and also in those cases, Default may be the better prefix.
There was a problem hiding this comment.
I'm using default because these are the functions/types that are used when the caller does not override them via options. In this sense, it matches DefaultErrorCallback, DefaultResponseCallback, and NewDefaultEventDispatcher, which already exist.
I'm open to renaming the scheduler if you have suggestions and then documenting the default-ness in comments. I'm less convinced that renaming the other functions will improve clarity.
|
|
||
| // ContextDeriver creates a new independent context from a request's context. | ||
| // The new context must be based on context.Background(), not the input. | ||
| type ContextDeriver func(context.Context) context.Context |
There was a problem hiding this comment.
why not have the Scheduler decide how to derive the context? then we could have
Scheduler.Schedule(ctx context.Context, dispatch Dispatch)
it feels like context deriving should be tied explicitly to schedule logic, rather than be swappable
There was a problem hiding this comment.
Context derivation is customizable because only the end user can know the context values that are relevant to copy to the new context for the handler, particularly if they've added custom middleware or other stuff before the event dispatcher. This felt like the right tradeoff to me, rather than having user implement their own schedulers to change the context, but maybe there are more elegant primitives to expose.
Regarding the signature, I was lazy: Dispatch contains the context so I could reuse the type in the internal queue for QueueAsysnScheduler. Looking at it now, I should probably make the context.Context a function argument and define a new private type for the queue.
Introduce a configurable scheduler option for NewEventDispatcher and add
two asynchronous schedulers, one that is unlimited and one that uses a
bounded queue and a fixed number of workers.
For many applications, switching to asynchronous handling will be as
easy as adding this new option when creating the event dispatcher. Other
application will need to provide a custom ContextDeriver to copy values
from the request context into the detached handler context.
As a result of error handling code introduced as part of this change,
go-githubapp now requires at least Go 1.13.