core (memory/sqla): Improve file_update by using raw value:

ORM was used, resulting in deserialisation/reserialisation of value checked for race
condition during update. In addition to the overhead, this could lead to modification even
if the value didn't change (e.g., because of JSON key order changed).

The raw value is not used to check if the row has not change between `select` and
`update`, making the method more robust.
This commit is contained in:
Goffi 2025-10-10 17:44:04 +02:00
commit e4e879d871

View file

@ -1112,12 +1112,12 @@ class Storage:
async def file_update(
self, file_id: str, column: str, update_cb: Callable[[dict], None]
) -> None:
"""Update a column value using a method to avoid race conditions
"""Update a column value using a method to avoid race conditions.
the older value will be retrieved from database, then update_cb will be applied to
The older value will be retrieved from database, then update_cb will be applied to
update it, and file will be updated checking that older value has not been changed
meanwhile by another user. If it has changed, it tries again a couple of times
before failing
before failing.
@param column: column name (only "access" or "extra" are allowed)
@param update_cb: method to update the value of the colum
the method will take older value as argument, and must update it in place
@ -1127,43 +1127,52 @@ class Storage:
"""
if column not in ("access", "extra"):
raise exceptions.InternalError("bad column name")
orm_col = getattr(File, column)
for i in range(5):
async with self.session() as session:
try:
value = (
await session.execute(select(orm_col).filter_by(id=file_id))
).scalar_one()
# We use raw SQL to bypass ORM processing and get the literal database
# value, which will be used for comparison on update: if the value has
# been changed between the select and update, the update will fail,
# preventing race condition.
stmt = text(f"SELECT {column} FROM files WHERE id = :file_id")
result = await session.execute(stmt, {"file_id": file_id})
row = result.first()
if not row:
raise NoResultFound()
raw_old_value = row[0]
# We deserialise for the callback.
new_value = {} if raw_old_value is None else json.loads(raw_old_value)
update_cb(new_value)
where_clause = text(f"id = :file_id AND {column} = :raw_old_value")
params = {"file_id": file_id, "raw_old_value": raw_old_value}
update_stmt = text(
f"UPDATE files SET {column} = :new_value WHERE "
+ where_clause.text
)
params["new_value"] = json.dumps(new_value, ensure_ascii=False)
result = await session.execute(update_stmt, params)
await session.commit()
except NoResultFound:
raise exceptions.NotFound
old_value = copy.deepcopy(value)
update_cb(value)
stmt = update(File).filter_by(id=file_id).values({column: value})
if not old_value:
# because JsonDefaultDict convert NULL to an empty dict, we have to
# test both for empty dict and None when we have an empty dict
stmt = stmt.where((orm_col == None) | (orm_col == old_value))
else:
stmt = stmt.where(orm_col == old_value)
result = await session.execute(stmt)
await session.commit()
if result.rowcount == 1:
break
if result.rowcount == 1:
break
log.warning(
_(
log.warning(
"table not updated, probably due to race condition, trying again "
"({tries})"
).format(tries=i + 1)
)
f"({i+1})."
)
else:
raise exceptions.DatabaseError(
_("Can't update file {file_id} due to race condition").format(
file_id=file_id
)
f"Can't update file {file_id} due to race condition"
)
@aio