-
-
Notifications
You must be signed in to change notification settings - Fork 309
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
[16.0][FIX] fastapi: Transactions handling refactoring #422
Conversation
2accaf3
to
3cfe00a
Compare
4b32c94
to
157c904
Compare
e39797f
to
8004371
Compare
@sbidoul @simahawk @qgroulard @AnizR @sebastienbeau This PR is a fundamental change in the way exceptions are handled in Fastapi's Odoo integration. It should be transparent but ensure a proper management of the transactions, environments and registry in case of exception. Before this change, for example, in some cases, some exceptions did not appear in the log files. This should no longer be the case, and the stack trace should be more relevant. The retry mechanism did not work as expected either. |
8004371
to
9cb8db9
Compare
Agree with the concept. I suppose you'll want to battle test it before declaring it ready to merge? |
The testsuite now ensures that the |
9cb8db9
to
c35ce48
Compare
|
by odoo and not by fastapi. We therefore need to remove all the handlers | ||
added by default when instantiating a FastAPI app. Since apps can be | ||
mounted recursively, we need to apply this method to all the apps in the | ||
mounted tree. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this needed when we mount a sub application too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this needed when we mount a sub application too?
The method is recursive. It walks across all the sub applications to clear the exception handlers. You shouldn't have to worry about this particular treatment.
c35ce48
to
bd8439c
Compare
This change fix an issue that prevented the system to retry a call to the application in case of a DB concurrency error if this error is defined as retryable. By default, any call to the application is retried in case of a DB concurrency if retryable. The 'retry' mechanism is implemented at the early stage of the processing of a request in case of specific postgreSQL errors. When a call is made to a fastapi application, the specific handler for the request is placed further down the stack. Before this change, any error that occurred in the specific fastapi handler was catch by the Fastapi processing stack and never bubbled up to the early stage of the processing where the 'retry' mechanism is implemented. This change fix this issue by putting in place a way to make some specific exceptions bubble up the fastapi processing stack through the event loop required to transform WSGI to ASGI.
This change is a complete rewrite of the way the transactions are managed when integrating a fastapi application into Odoo. In the previous implementation, specifics error handlers were put in place to catch exception occurring in the handling of requests made to a fastapi application and to rollback the transaction in case of error. This was done by registering specifics error handlers methods to the fastapi application using the 'add_exception_handler' method of the fastapi application. In this implementation, the transaction was rolled back in the error handler method. This approach was not working as expected for several reasons: - The handling of the error at the fastapi level prevented the retry mechanism to be triggered in case of a DB concurrency error. This is because the error was catch at the fastapi level and never bubbled up to the early stage of the processing of the request where the retry mechanism is implemented. - The cleanup of the environment and the registry was not properly done in case of error. In the **'odoo.service.model.retrying'** method, you can see that the cleanup process is different in case of error raised by the database and in case of error raised by the application. This change fix these issues by ensuring that errors are no more catch at the fastapi level and bubble up the fastapi processing stack through the event loop required to transform WSGI to ASGI. As result the transactional nature of the requests to the fastapi applications is now properly managed by the Odoo framework.
bd8439c
to
05c663e
Compare
/ocabot merge minor |
On my way to merge this fine PR! |
Congratulations, your PR was merged at a64cfa7. Thanks a lot for contributing to OCA. ❤️ |
This change is a complete rewrite of the way the transactions are managed in the
when integrating a fastapi application into Odoo.
In the previous implementation, specifics error handlers were put in place to
catch exception occurring in the handling of requests made to a fastapi application
and to rollback the transaction in case of error. This was done by registering
specifics error handlers methods to the fastapi application using the 'add_exception_handler'
method of the fastapi application. In this implementation, the transaction was
rolled back in the error handler method.
This approach was not working as expected for several reasons:
to be triggered in case of a DB concurrency error. This is because the error
was catch at the fastapi level and never bubbled up to the early stage of the
processing of the request where the retry mechanism is implemented.
of error. In the 'odoo.service.model.retrying' method, you can see that
the cleanup process is different in case of error raised by the database
and in case of error raised by the application.
This change fix these issues by ensuring that errors are no more catch at the
fastapi level and bubble up the fastapi processing stack through the event loop
required to transform WSGI to ASGI. As result the transactional nature of the
requests to the fastapi applications is now properly managed by the Odoo framework.