From 869a7339e88dde3ec39864b33f96a7d941139125 Mon Sep 17 00:00:00 2001 From: Mart Pluijmaekers Date: Fri, 28 Feb 2025 15:08:38 +0100 Subject: [PATCH 1/4] Lock, and transaction-free entry_point update --- .../Controller/API/JudgehostController.php | 85 ++++++++++++++++--- 1 file changed, 71 insertions(+), 14 deletions(-) diff --git a/webapp/src/Controller/API/JudgehostController.php b/webapp/src/Controller/API/JudgehostController.php index 5f9523ff4e..3a9d765b68 100644 --- a/webapp/src/Controller/API/JudgehostController.php +++ b/webapp/src/Controller/API/JudgehostController.php @@ -32,6 +32,7 @@ use Doctrine\DBAL\ArrayParameterType; use Doctrine\DBAL\Exception; use Doctrine\DBAL\Exception as DBALException; +use Doctrine\DBAL\TransactionIsolationLevel; use Doctrine\ORM\AbstractQuery; use Doctrine\ORM\EntityManager; use Doctrine\ORM\EntityManagerInterface; @@ -313,33 +314,89 @@ public function updateJudgingAction( } if ($request->request->has('output_compile')) { + $output_compile = base64_decode($request->request->get('output_compile')); + // Note: we use ->get here instead of ->has since entry_point can be the empty string and then we do not // want to update the submission or send out an update event if ($request->request->get('entry_point')) { - $this->em->wrapInTransaction(function () use ($query, $request, &$judging) { - $submission = $judging->getSubmission(); - if ($submission->getEntryPoint() === $request->request->get('entry_point')) { - return; + // Lock-free setting of, and detection of mismatched entry_point. + $submission = $judging->getSubmission(); + + // Retrieve, and update the current entrypoint. + $oldEntryPoint = $submission->getEntryPoint(); + $newEntryPoint = $request->request->get('entry_point'); + + + if ($oldEntryPoint === $newEntryPoint) { + // Nothing to do + } elseif (!empty($oldEntryPoint)) { + // Conflict detected disable the judgehost. + $disabled = [ + 'kind' => 'judgehost', + 'hostname' => $judgehost->getHostname(), + ]; + $error = new InternalError(); + $error + ->setJudging($judging) + ->setContest($judging->getContest()) + ->setDescription('Reported EntryPoint conflict difference for j' . $judging->getJudgingid().'. Expected: "' . $oldEntryPoint. '", received: "' . $newEntryPoint . '".') + ->setJudgehostlog(base64_encode('New compilation output: ' . $output_compile)) + ->setTime(Utils::now()) + ->setDisabled($disabled); + $this->em->persist($error); + } else { + // Update needed. Note, conflicts might still be possible. + + $rowsAffected = $this->em->createQueryBuilder() + ->update(Submission::class, 's') + ->set('s.entry_point', ':entrypoint') + ->andWhere('s.submitid = :id') + ->andWhere('s.entry_point IS NULL') + ->setParameter('entrypoint', $newEntryPoint) + ->setParameter('id', $submission->getSubmitid()) + ->getQuery() + ->execute(); + + if ($rowsAffected == 0) { + // There is a potential conflict, two options. + // The new entry point is either the same (no issue) or different (conflict). + // Read the entrypoint and check. + $this->em->clear(); + $currentEntryPoint = $query->getOneOrNullResult()->getSubmission()->getEntryPoint(); + if ($newEntryPoint !== $currentEntryPoint) { + // Conflict detected disable the judgehost. + $disabled = [ + 'kind' => 'judgehost', + 'hostname' => $judgehost->getHostname(), + ]; + $error = new InternalError(); + $error + ->setJudging($judging) + ->setContest($judging->getContest()) + ->setDescription('Reported EntryPoint conflict difference for j' . $judging->getJudgingid().'. Expected: "' . $oldEntryPoint. '", received: "' . $newEntryPoint . '".') + ->setJudgehostlog(base64_encode('New compilation output: ' . $output_compile)) + ->setTime(Utils::now()) + ->setDisabled($disabled); + $this->em->persist($error); + } + } else { + $submissionId = $submission->getSubmitid(); + $contestId = $submission->getContest()->getCid(); + $this->eventLogService->log('submission', $submissionId, + EventLogService::ACTION_UPDATE, $contestId); } - $submission->setEntryPoint($request->request->get('entry_point')); - $this->em->flush(); - $submissionId = $submission->getSubmitid(); - $contestId = $submission->getContest()->getCid(); - $this->eventLogService->log('submission', $submissionId, - EventLogService::ACTION_UPDATE, $contestId); - // As EventLogService::log() will clear the entity manager, so the judging has - // now become detached. We will have to reload it. + // As EventLogService::log() will clear the entity manager, both branches clear the entity manager. + // The judging is now detached, reload it. /** @var Judging $judging */ $judging = $query->getOneOrNullResult(); - }); + } } // Reload judgehost just in case it got cleared above. /** @var Judgehost $judgehost */ $judgehost = $this->em->getRepository(Judgehost::class)->findOneBy(['hostname' => $hostname]); - $output_compile = base64_decode($request->request->get('output_compile')); if ($request->request->getBoolean('compile_success')) { if ($judging->getOutputCompile() === null) { $judging From 4b827d8696a6ab63caa2b22b3215fc109759377d Mon Sep 17 00:00:00 2001 From: Mart Pluijmaekers Date: Fri, 28 Feb 2025 16:25:35 +0100 Subject: [PATCH 2/4] Transaction free update of filtered internalError update --- .../Controller/API/JudgehostController.php | 44 ++++++++----------- 1 file changed, 18 insertions(+), 26 deletions(-) diff --git a/webapp/src/Controller/API/JudgehostController.php b/webapp/src/Controller/API/JudgehostController.php index 3a9d765b68..51db10877b 100644 --- a/webapp/src/Controller/API/JudgehostController.php +++ b/webapp/src/Controller/API/JudgehostController.php @@ -866,32 +866,24 @@ public function internalErrorAction(Request $request): ?int if ($field_name !== null) { // Disable any outstanding judgetasks with the same script that have not been claimed yet. - $this->em->wrapInTransaction(function (EntityManager $em) use ($field_name, $disabled_id, $error) { - $judgingids = $em->getConnection()->executeQuery( - 'SELECT DISTINCT jobid' - . ' FROM judgetask' - . ' WHERE ' . $field_name . ' = :id' - . ' AND judgehostid IS NULL' - . ' AND valid = 1', - [ - 'id' => $disabled_id, - ] - )->fetchFirstColumn(); - $judgings = $em->getRepository(Judging::class)->findBy(['judgingid' => $judgingids]); - foreach ($judgings as $judging) { - /** @var Judging $judging */ - $judging->setInternalError($error); - } - $em->flush(); - $em->getConnection()->executeStatement( - 'UPDATE judgetask SET valid=0' - . ' WHERE ' . $field_name . ' = :id' - . ' AND judgehostid IS NULL', - [ - 'id' => $disabled_id, - ] - ); - }); + $rows = $this->em->createQueryBuilder() + ->update(Judging::class, 'j') + ->leftJoin(JudgeTask::class, 'jt') + ->set('j.internal_error', ':error') + ->set('jt.valid', 0) + ->andWhere('jt.' . $field_name . ' = :id') + ->andWhere('j.internal_error IS NULL') + ->andWhere('jt.judgehost_id IS NULL') + ->andWhere('jt.valid = 1') + ->setParameter('error', $error) + ->setParameter('id', $disabled_id) + ->distinct() + ->getQuery() + ->getArrayResult(); + + if ($rows == 0) { + // TODO, handle this case. Nothing was updated. + } } $this->dj->setInternalError($disabled, $contest, false); From 00f98e2d0771df287314bc12e9f1067cae3d81bc Mon Sep 17 00:00:00 2001 From: Mart Pluijmaekers Date: Fri, 28 Feb 2025 17:18:35 +0100 Subject: [PATCH 3/4] Do not use transaction while handing back judging --- .../Controller/API/JudgehostController.php | 50 +++++++++---------- 1 file changed, 24 insertions(+), 26 deletions(-) diff --git a/webapp/src/Controller/API/JudgehostController.php b/webapp/src/Controller/API/JudgehostController.php index 51db10877b..0300740921 100644 --- a/webapp/src/Controller/API/JudgehostController.php +++ b/webapp/src/Controller/API/JudgehostController.php @@ -910,37 +910,22 @@ public function internalErrorAction(Request $request): ?int */ protected function giveBackJudging(int $judgingId, ?Judgehost $judgehost): void { + // Reset the judgings without using Doctrine, it has no support for update queries containing a join. + // Both databases supported by DOMjudge (MariaDB, MySQL) support these types of queries. $judging = $this->em->getRepository(Judging::class)->find($judgingId); if ($judging) { - $this->em->wrapInTransaction(function () use ($judging, $judgehost) { - /** @var JudgingRun $run */ - foreach ($judging->getRuns() as $run) { - if ($judgehost === null) { - // This is coming from internal errors, reset the whole judging. - $run->getJudgetask() - ->setValid(false); - continue; - } - - // We do not have to touch any finished runs - if ($run->getRunresult() !== null) { - continue; - } - - // For the other runs, we need to reset the judge task if it belongs to the current judgehost. - if ($run->getJudgetask()->getJudgehost() && $run->getJudgetask()->getJudgehost()->getHostname() === $judgehost->getHostname()) { - $run->getJudgetask() - ->setJudgehost(null) - ->setStarttime(null); - } - } - - $this->em->flush(); - }); - if ($judgehost === null) { // Invalidate old judging and create a new one - but without judgetasks yet since this was triggered by // an internal error. + $this->em->getConnection()->executeStatement( + 'UPDATE judging_run jr ' . + 'JOIN judgetask jt ON jt.judgetaskid = jr.judgetaskid ' . + 'SET jt.valid = 0 ' . + 'WHERE jr.judgingid = :judgingid', + [ + 'judgingid' => $judgingId, + ]); + $judging->setValid(false); $newJudging = new Judging(); $newJudging @@ -950,6 +935,19 @@ protected function giveBackJudging(int $judgingId, ?Judgehost $judgehost): void ->setOriginalJudging($judging); $this->em->persist($newJudging); $this->em->flush(); + } else { + // Hand back the non-completed work of this judgehost within this judgetask. + $this->em->getConnection()->executeStatement( + 'UPDATE judging_run jr ' . + 'JOIN judgetask jt ON jt.judgetaskid = jr.judgetaskid ' . + 'SET jt.judgehostid = null, jt.starttime = null ' . + 'WHERE jr.judgingid = :judgingid ' . + ' AND jr.runresult IS NOT NULL ' . + ' AND jt.judgehostid = :judgehost', + [ + 'judgingid' => $judgingId, + 'judgehost' => $judgehost->getJudgehostid(), + ]); } $this->dj->auditlog('judging', $judgingId, 'given back' From 6861dd1b2f53c5d7e62e64677ef6e219e160eae2 Mon Sep 17 00:00:00 2001 From: Mart Pluijmaekers Date: Sat, 5 Jul 2025 18:11:14 +0200 Subject: [PATCH 4/4] Mark transaction as valid use. Tried removing this transaction for the removal of all unneccesary transactions. It turns out that this is actually a valid usecase that should be kept. --- webapp/src/Controller/BaseController.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/webapp/src/Controller/BaseController.php b/webapp/src/Controller/BaseController.php index 474d093d76..27d1b5fe15 100644 --- a/webapp/src/Controller/BaseController.php +++ b/webapp/src/Controller/BaseController.php @@ -239,6 +239,9 @@ protected function commitDeleteEntity(object $entity, array $primaryKeyData): vo } // Now actually delete the entity. + // This is a great use of a transaction! Order needs to be guaranteed. Normally deletion + // succeeds, in cases where it does not, all work executed by the deletion must be restored + // to bring the database in an expected and consistent state. $this->em->wrapInTransaction(function () use ($entity) { if ($entity instanceof Problem) { // Deleting a problem is a special case: