-
Notifications
You must be signed in to change notification settings - Fork 24
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
Proposal - use asyncio for events management (Supporting Event Chaining) #23
Comments
@ndopj Thank you very much for the proposition and your analysis. I totally agree with you when it comes to the limitation of the current implementation. Event chaining was something I was thinking of when I started the project but haven't spent too much effort into it as it hasn't bit me yet. While your proposal seems feasible, it is definitely a breaking change as you described. If we were to introduce a new architecture (which I think we should), here's what I think we can do:
Please let me know your thoughts. Any proposals to the API are also largely welcome 🙏 |
@ndopj I believe there is another issue we need to solve if we want to support an architecture without middleware. Today, handlers are specified as arguments when adding the middleware:
or
Without the middleware, we need another solution to allow users to specify the handlers they use. So that all handlers work properly with the asyncio event loop architecture:
I don't have a good idea yet. It seems like we will need to leverage global context or similar for this purpose. Any suggestions are welcome too |
@melvinkcx I agree on mentioned two steps of how to introduce the new architecture. Such solution is backwards compatible and by default provides users with expected current architecture based behavior. Also if anywhere in the future decision will be made to stick with either event loop or middleware its easy to discard the other architecture (let's say when there will be a new major release version with other changes breaking old behavior). Also we should use Enum instead of plain strings for Regarding all handlers registration (not to be confused with event handler registration) we should first raise a valid points for what we want to achieve from new registration system (also note from here that I am nowhere close to experienced python developer and my development experience origins in different languages).
I think that we can agree that global context solution is fallback option here for us. Note that for now there will always be need for middle-ware registration if current architecture is expected from user so this is only about how
While this might be solution we should at least try to find more cleaner approach. First thing that I come up with was to use fastapi dependencies system. But this would anyway require to manually register handler(s) to global store, so in the end this can be used just to write cleaner code with our fallback option. Another thing would be to somehow obtain registered middle-ware from fastapi so we can access all registered handlers even from event loop dispatched event. While this does maintain backwards compatibility it would require users using event loop to register middle-ware with specified event handlers despite not using middle-ware arch. so middle-ware would act only as registry for handler(s). This might be confusing to users and is probably also no go solution. Last thing in my mind is to self register handler to the global context (similar to proposed fallback solution) in the handler base class. Since all handlers must be instantiated so users can register event processing functions we could modify base class in such way that in the constructor it would register itself to the global context store. Middle-ware and event loop events would then use this global store to access all handlers. Since this would automatically register all handlers, handlers passed to Library part:
User part:
This way all the handlers would be accessed from the global context so even event loop events can access handlers registered trough middle-ware (current code bases) while in the mentioned fallback with manual registration this is not the case (unless modified little bit). If this proposal would change to implementation I would suggest to first implement automatic handler registration to ensure this will work. On top of that it should be easy to build solution with event loop dispatching and the system you mentioned to preserve backwards compatibility forcing by default current behavior . |
@ndopj Absolutely right about using enums! Let me separate my response into sections to make it easier to consume. GoalsBefore diving into the implementation details, I think we should first agree on the goals. Here are what I think:
Backward CompatibilityTo achieve goal#2 from the above, I think we should preserve these behaviours:
Achieving Event ChainingThe main idea of your proposal is to achieve event chaining, and we both agree that an architectural change is necessary. In order to preserve the behaviours outlined in the section "Backward Compatibility", I'm exploring the possibility of using a hybrid approach (event loop + middleware + global context). Here's my idea:
With the above, I believe all the goals described will be achieved. About Auto-Registering HandlersSimilar to how the isolation of middleware instances might be needed for certain use cases, such as testing. Implementing auto-registration upon instantiation might be too much of a side effect as it will drastically change the behaviour and there isn't a way to opt out either. I think we can revisit this idea in the future if there are more compelling arguments for us to introduce this. Let me know what you think =) |
This has been implemented in #24 |
fastapi-events currently uses ASGI middle-ware for the events management. This middle-ware creates event store for each specific request which is filled by events dispatched while computing the request and then used to execute collected events after the response was sent to a client. While this might be sufficient solution, this architecture has some disadvantages and there might be even more simplistic solution.
Initial problem
First of all I am really thankful for this library and great work put into it. One of the limitations of currently used architecture and also reason why I had to stick with custom solution was the fact that currently its not possible to dispatch event from a registered handler (not possible to chain handlers by event). Since
dispatch()
function called from registered handler is called after the response was already sent there is no middle-ware which would execute additional events.Custom simplistic solution
It took me some time to find out how would I create custom self-executing event queue which would execute events no matter when and where dispatched as long as
dispatch()
function has access to event queue itself. Then I got an idea that if FastAPI is built on top of asyncio it should definitely be possible to dispatch tasks/events to the event loop so it will be queued together with other FastAPI tasks (mainly request handlers?). Following is very simple code change that allows to dispatch events into asyncio event loop and therefore there is not any requirement for event store, middle-ware executor and handling more than one task at a time.Differences between task management architectures
There are some key points to consider from the table above. While both strategies don't block the response, strategy with asyncio event loop can execute event sooner then the response is sent to client. This might happen when we do
dispatch(event)
with consecutiveawait promise
. The event is dispatched to the event loop but since there is await after event has been dispatched the event loop might start executing dispatched event. From user perspective I would say this is acceptable/preferable behavior - I have already dispatched event but still awaiting for other resource and therefore other tasks can be executed in mean time. If dispatch is called and there is no consecutive await its guaranteed that it will be executed after the current event(request) finishes its execution.While this change in architecture might break behavior users are used to I would say that strategy of detaching events execution to the asyncio event pool is more preferred and stable for the future. Instead of executing event right after the request/response where it was dispatched, event is sent to a queue and scheduled for execution with every other request/response and events that Fastapi creates. New architecture still allows old behavior to be used in case anyone needs to schedule event execution after the response. Moreover this architecture allows users to define preferred behavior instead of forcing them with strict rules.
The text was updated successfully, but these errors were encountered: