mirror of
https://github.com/ZoneMinder/zoneminder.git
synced 2026-09-13 05:38:05 -04:00
Isaac ConnorandClaude Opus 5
1f70965a98
zmDbDo, zmDbDoInsert and zmDbDoUpdate all decided what to do with a failed query by testing for ER_LOCK_WAIT_TIMEOUT alone, which got both halves of lock contention wrong: - ER_LOCK_DEADLOCK was not retried at all. InnoDB resolves a deadlock by rolling one side back and expects that side to re-run; instead the query was logged and abandoned. Event creation goes through zmDbDoInsert, which is where this actually bites. - ER_LOCK_WAIT_TIMEOUT re-ran immediately and forever, with no delay and no attempt limit, so two writers deadlocking against each other kept colliding on the same schedule. Both now go through one retry decision: five attempts with a jittered 50ms-doubling backoff, then give up and report. The jitter is what stops two contending sessions waking together and repeating the deadlock. The wait happens under db_mutex, which every other database user in the process is blocked on, so the budget is deliberately small -- about 3.1s across all five attempts. That is still far less than the unbounded ER_LOCK_WAIT_TIMEOUT loop it replaces, where each round costs a full innodb_lock_wait_timeout. The change in behaviour is that a query which would eventually have won after many minutes is now abandoned; it is reported at Error with the attempt count. The error is now logged once, when giving up, rather than on every round. mysql_errno is read next to mysql_error rather than after the logging call, so it cannot be clobbered in between. Two other things, both small and both in the same area: zmDbEscapeString called mysql_real_escape_string unconditionally, and that reads the character set off the connection, so a closed handle sends it into freed state. It now falls back to escaping the injection-relevant characters itself. That fallback is only correct because the connection is utf8mb4, where no byte of a multi-byte sequence is ASCII and so no sequence can absorb a trailing backslash; the comment says so, since it would be wrong for a character set like GBK. It deliberately does not take db_mutex to read the flag: the logger calls this from Error(), and zmDbFetch reaches Error() while holding db_mutex, so locking here would self-deadlock the process on any failed query. zm_rtsp_server was the one daemon closing the database without stopping the queue that writes through it. Fixed at the call site rather than inside zmDbClose, which holds db_mutex while the queue thread needs that same mutex to drain -- joining it from in there would deadlock. Tests: tests/zm_db_contention.cpp covers the retry budget only, 2011 assertions. Verified it fails when the budget is removed. The retry loop itself needs a database and two contending sessions and is NOT covered; it wants verifying against a real server under contention before this is relied on. Full suite 12171 assertions in 133 test cases. Builds clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Y6FieTwEXuLhhR4e2yiax
68 lines
2.8 KiB
C++
68 lines
2.8 KiB
C++
/*
|
|
* This file is part of the ZoneMinder Project. See AUTHORS file for Copyright information
|
|
*
|
|
* This program is free software; you can redistribute it and/or modify it
|
|
* under the terms of the GNU General Public License as published by the
|
|
* Free Software Foundation; either version 2 of the License, or (at your
|
|
* option) any later version.
|
|
*
|
|
* This program is distributed in the hope that it will be useful, but WITHOUT
|
|
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
|
|
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
|
|
* more details.
|
|
*
|
|
* You should have received a copy of the GNU General Public License along
|
|
* with this program. If not, see <http://www.gnu.org/licenses/>.
|
|
*/
|
|
|
|
#include "zm_catch2.h"
|
|
#include "zm_db.h"
|
|
|
|
// Covers the retry budget only. The retry loop itself needs a database and two
|
|
// sessions contending for the same rows, so it is not exercised here.
|
|
TEST_CASE("zmDbContentionBackoff", "[db]") {
|
|
SECTION("the budget is finite") {
|
|
// The point of the schedule: a query that keeps losing lock races is
|
|
// eventually abandoned. Before this, a lock wait timeout re-ran forever.
|
|
REQUIRE(zmDbContentionBackoff(kMaxDbContentionRetries) > 0);
|
|
REQUIRE(zmDbContentionBackoff(kMaxDbContentionRetries + 1) == 0);
|
|
REQUIRE(zmDbContentionBackoff(kMaxDbContentionRetries + 100) == 0);
|
|
}
|
|
|
|
SECTION("attempt numbering starts at one") {
|
|
REQUIRE(zmDbContentionBackoff(1) > 0);
|
|
REQUIRE(zmDbContentionBackoff(0) == 0);
|
|
REQUIRE(zmDbContentionBackoff(-1) == 0);
|
|
}
|
|
|
|
SECTION("each attempt waits longer than the one before") {
|
|
// Jittered, so compare across the gap rather than sampling twice and
|
|
// assuming the same value comes back.
|
|
for (int attempt = 1; attempt < kMaxDbContentionRetries; attempt++) {
|
|
REQUIRE(zmDbContentionBackoff(attempt) < zmDbContentionBackoff(attempt + 1));
|
|
}
|
|
}
|
|
|
|
SECTION("jitter stays inside its band") {
|
|
// Two callers that deadlocked against each other must not wake together and
|
|
// do it again, so the wait is spread, but only within the attempt's slot.
|
|
for (int attempt = 1; attempt <= kMaxDbContentionRetries; attempt++) {
|
|
useconds_t base = 50000u * (1u << attempt);
|
|
for (int sample = 0; sample < 200; sample++) {
|
|
useconds_t backoff = zmDbContentionBackoff(attempt);
|
|
REQUIRE(backoff >= base);
|
|
REQUIRE(backoff < base + 50000u);
|
|
}
|
|
}
|
|
}
|
|
|
|
SECTION("the whole budget is a bounded stall") {
|
|
// The sleep happens under db_mutex, which every other database user in the
|
|
// process is waiting on, so the total has to stay small.
|
|
useconds_t total = 0;
|
|
for (int attempt = 1; attempt <= kMaxDbContentionRetries; attempt++)
|
|
total += zmDbContentionBackoff(attempt);
|
|
REQUIRE(total < 4000000u); // under 4 seconds
|
|
}
|
|
}
|