Opened 3 years ago

Closed 2 years ago

#22967 closed Bug (fixed)

Model._do_update not always consistent with the precise meaning of returned value

Reported by: knaperek Owned by: knaperek
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: select_on_save concurrency race-condition
Cc: knaperek Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


This is a bit obscure corner-case bug which might seem unimportant, but the solution is easy and actually only makes the code simpler (removes one nested if) and more consistent.

When select_on_save is set, Model._do_update() ignores the result of Model._update() if it learnt that the object exists in a previous DB call. That last thing is crucial, as the object could have already been deleted by concurrent worker just in-between these two ORM calls (exists and _update). In such case, the _update() call will fail, resulting in nothing being actually updated. However, _do_update() method will currently return True regardless of _update() result, which is inconsistent with the common (and expected) behavior, and also in contrast with the method's docs (saying: If the model was updated (in the sense that an update query was done and a matching row was found from the DB) the method will return True.)

As a consequence, object will not be saved into the database, because _save_table() won't call _do_insert(), being misled by _do_update(). Now you may oppose that this does not really matter that much, since it has been caused by concurrent modifications of the same data, which is considered non-deterministic anyway (so noone has the right to complain that the object is not there at the end). However, here's one example that vouches for this bug to be fixed: if you happen to have a trigger in your DB counting the number of times an object was changed (updated/inserted), you'd be missing one (which is no longer justifiable by concurrent save() and delete() operations).

As it sometimes happens, it is IMHO impossible to create a unit test for this bug, as it is too low level thing, exploitable only in a concurrent environment, and with some (bad) luck. I only found the bug when studying the code. So that's why I tried to provide a solid explanation, believing the fix I'm about to make should be obvious and mergable without worries.

Change History (8)

comment:1 Changed 3 years ago by knaperek

Owner: changed from nobody to knaperek
Status: newassigned

comment:2 Changed 3 years ago by knaperek

My fix is available in this Github branch

comment:3 Changed 3 years ago by knaperek

Has patch: set

comment:4 Changed 3 years ago by knaperek

I've modified the fix to count with the rare DB behavior when a successful UPDATE returns zero (as discussed in #20988).

More info in Github discussion.

comment:5 Changed 3 years ago by knaperek

Keywords: select_on_save concurrency race-condition added
Version: 1.6master

comment:6 Changed 2 years ago by Tim Graham

Triage Stage: UnreviewedAccepted

On the PR Anssi said at least docs and comments should be updated.

comment:7 Changed 2 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

Anssi will commit this.

comment:8 Changed 2 years ago by Anssi Kääriäinen <akaariai@…>

Resolution: fixed
Status: assignedclosed

In c56c42b5c01ed774cfa8e044cd372a984608536c:

Fixed #22967 -- Made Model._do_update consistent

Made _do_update behave more strictly according to its docs,
including a corner case when specific concurent updates are
executed and select_on_save is set.

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