Opened 2 years ago

Closed 19 months ago

Last modified 19 months ago

#21166 closed Bug (fixed)

Reset the errors_occurred flag

Reported by: aaugustin Owned by: aaugustin
Component: Database layer (models, ORM) Version: 1.6-beta-1
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


Currently it's reset only in close_if_unusable_or_obsolete, after making a test query.

I think it should be reset in atomic.__exit__ after a successful rollback too.

Reported by Anssi.

Change History (5)

comment:1 Changed 2 years ago by aaugustin

  • Owner changed from nobody to aaugustin
  • Status changed from new to assigned

comment:2 Changed 20 months ago by aaugustin

In fact, I'm hesitating about this. It's a very small change:

errors_occurred is set after exceptions other than DataError and IntegrityError. Given the definition of these two exceptions, they're unlikely to be thrown in situations that make the connection unusable. I can't say much about the other exceptions. That's why I implemented a safety net in close_if_unusable_or_obsolete(). When another exception occurs, that method tests if the connection still works, and if it doesn't, drops it.

The vast majority of expected and recoverable exceptions are IntegrityErrors. These don't set the flag and don't incur the cost of the extra query to check the state of the connection. So there isn't a lot at stake.

But a small gain is still a gain. I believe that a successful ROLLBACK validates the state of the connection just as well as a SELECT 1. While we're there, a successful COMMIT also means that the connection works.

The argument against this change is to keep the safety net simple. Preserving a broken connection is very bad; making an extra query on a working connection is almost negligible.

Considering that the patch is dead simple, if I had to make the choice, I'd say +0.

comment:3 Changed 20 months ago by aaugustin

  • Has patch set

comment:4 Changed 19 months ago by Aymeric Augustin <aymeric.augustin@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 3033a7193afcd1abdc0158e95ca3a241d8448d25:

Fixed #21166 -- Reset errors_occurred flag after commit and rollback.

comment:5 Changed 19 months ago by Aymeric Augustin <aymeric.augustin@…>

In 6b38e48ba11ec3d5aa433948d1354a8627c1edba:

[1.7.x] Fixed #21166 -- Reset errors_occurred flag after commit and rollback.

Backport of 3033a71 from master.



Note: See TracTickets for help on using tickets.
Back to Top