Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#21166 closed Bug (fixed)

Reset the errors_occurred flag

Reported by: Aymeric Augustin Owned by: Aymeric Augustin
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 5 years ago by Aymeric Augustin

Owner: changed from nobody to Aymeric Augustin
Status: newassigned

comment:2 Changed 5 years ago by Aymeric Augustin

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 5 years ago by Aymeric Augustin

Has patch: set

comment:4 Changed 5 years ago by Aymeric Augustin <aymeric.augustin@…>

Resolution: fixed
Status: assignedclosed

In 3033a7193afcd1abdc0158e95ca3a241d8448d25:

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

comment:5 Changed 5 years 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