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

[16.0][FIX] fastapi: Transactions handling refactoring #422

Open
wants to merge 5 commits into
base: 16.0
Choose a base branch
from

Conversation

lmignon
Copy link
Sponsor Contributor

@lmignon lmignon commented Mar 15, 2024

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:

  • 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.

@lmignon lmignon marked this pull request as draft March 18, 2024 09:57
@lmignon lmignon force-pushed the 16.0-fastapi-fix-retrying branch 4 times, most recently from 4b32c94 to 157c904 Compare March 19, 2024 14:10
@lmignon lmignon changed the title [16.0][FIX] fastapi: Enable auto retry of calls in case of concurrent update. [16.0][FIX] fastapi: Transactions handling refactoring Mar 19, 2024
@lmignon lmignon marked this pull request as ready for review March 19, 2024 14:12
@lmignon lmignon marked this pull request as draft March 19, 2024 15:32
@lmignon lmignon force-pushed the 16.0-fastapi-fix-retrying branch 2 times, most recently from e39797f to 8004371 Compare March 20, 2024 08:19
@lmignon lmignon marked this pull request as ready for review March 20, 2024 08:23
@lmignon
Copy link
Sponsor Contributor Author

lmignon commented Mar 20, 2024

@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.

@sbidoul
Copy link
Member

sbidoul commented Mar 20, 2024

Agree with the concept. I suppose you'll want to battle test it before declaring it ready to merge?

@lmignon
Copy link
Sponsor Contributor Author

lmignon commented Mar 20, 2024

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 commit method is not called in the case of an exception, unlike normal calls. I'm going to do some additional tests on my project but by running all the tests in the debugger I've been able to check that whatever exception is thrown, it goes back into Odoo's retrying method which manages the transactional aspect of the calls. The dispatcher also handles the formatting of the response according to the type of error.

@lmignon lmignon marked this pull request as draft April 19, 2024 13:38
@lmignon
Copy link
Sponsor Contributor Author

lmignon commented Apr 19, 2024

Found issue.... will fix
Issue no in the base lib but in my endpoint implementation.... a 'return HttpException in place of araise HttpException 😵‍💫

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.
Copy link
Contributor

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?

Copy link
Sponsor Contributor Author

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.

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.
@lmignon lmignon marked this pull request as ready for review April 19, 2024 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants