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

Async Eventloop #1474

Merged
merged 18 commits into from
Apr 8, 2021
Merged

Async Eventloop #1474

merged 18 commits into from
Apr 8, 2021

Conversation

iancooper
Copy link
Member

This PR is intended to support us having async dispatcher pipelines again. Obviously, these need documentation because they have side efffects developers may not understand such as Requests lose guarantee of processing "in order"

There is a lot to consider here:

  • Do we want threadpool based callbacks?

    -- This may be the easiest to get 'working'

    -- We now have an issue with backpressure, because the async dispatcher will keep eating work from the queue whilst it is waiting for non-blocking I/O to complete, which could end up with a lot of work queued if we support using the threadpool for callbacks.

    -- Leads to "max in flight" solutions that increment and decrement a counter for number of messages before we pause reading the queue

  • Alternatively, do we want a single threaded event loop

    -- Easier to apply backpressure - only allow so many pending events in our queue

    -- Thread affinity fixes some issues, such as HttpContext

    -- Won't get you the same throughput, because the continuation will await the next message hitting non-blocking I/O or finishing with that message

Our preference is for the latter model, because it should be simpler to reason about, but it requires a custom synchronization context.

@iancooper
Copy link
Member Author

So I think this may get async pipelines working, running continuations on the same thread, but there is a fair amount of testing to do still. However, @preardon if you wanted to play by flipping the runAsync flag on your subscription to true and see if it explodes, its a starting point.
@holytshirt just FYI for now

@iancooper iancooper marked this pull request as draft March 31, 2021 16:52
@iancooper iancooper marked this pull request as ready for review April 6, 2021 16:46
@holytshirt holytshirt merged commit ca38d91 into master Apr 8, 2021
@iancooper iancooper deleted the async_eventloop branch April 8, 2021 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants