From 175ab4cefe37a6d6565801fe38856f8f6ce4fce8 Mon Sep 17 00:00:00 2001 From: mhsdesign <85400359+mhsdesign@users.noreply.github.com> Date: Tue, 3 Dec 2024 15:18:03 +0100 Subject: [PATCH] Move savepoint creation back on the subscription store This reverts commit dc5ff1038b89ca656eb01a420e4763059b455f70. We discussed that we dont want to ensure exactly once delivery for external projections. Adding transaction logic via traits for the projections increases logic there. And while with alot of effort we could bring exactly once delivery to work for most cases php could still decide to die in the small timeframe where we commit the one transaction and then the second. So there is no gurantee except when using a dedicated store: https://github.com/neos/neos-development-collection/pull/5377 --- .../DoctrineDbalContentGraphProjection.php | 3 -- .../Projection/HypergraphProjection.php | 3 -- .../TestSuite/DebugEventProjection.php | 3 -- .../AbstractSubscriptionEngineTestCase.php | 1 - .../ProjectionTransactionTrait.php | 37 ------------------- .../Projection/ProjectionInterface.php | 10 ----- .../Engine/SubscriptionEngine.php | 9 ++++- .../Store/SubscriptionStoreInterface.php | 6 +++ .../Subscriber/ProjectionSubscriber.php | 8 ++-- .../DoctrineSubscriptionStore.php | 15 ++++++++ .../Projection/DocumentUriPathProjection.php | 3 -- .../ChangeProjection.php | 3 -- 12 files changed, 31 insertions(+), 70 deletions(-) delete mode 100644 Neos.ContentRepository.Core/Classes/Infrastructure/ProjectionTransactionTrait.php diff --git a/Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php b/Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php index 025f596f5c..4e998fde99 100644 --- a/Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php +++ b/Neos.ContentGraph.DoctrineDbalAdapter/src/DoctrineDbalContentGraphProjection.php @@ -62,7 +62,6 @@ use Neos\ContentRepository\Core\Feature\WorkspaceRebase\Event\WorkspaceRebaseFailed; use Neos\ContentRepository\Core\Feature\WorkspaceRebase\Event\WorkspaceWasRebased; use Neos\ContentRepository\Core\Infrastructure\DbalSchemaDiff; -use Neos\ContentRepository\Core\Infrastructure\ProjectionTransactionTrait; use Neos\ContentRepository\Core\NodeType\NodeTypeName; use Neos\ContentRepository\Core\Projection\ContentGraph\ContentGraphProjectionInterface; use Neos\ContentRepository\Core\Projection\ContentGraph\ContentGraphReadModelInterface; @@ -81,8 +80,6 @@ */ final class DoctrineDbalContentGraphProjection implements ContentGraphProjectionInterface { - use ProjectionTransactionTrait; - use ContentStream; use NodeMove; use NodeRemoval; diff --git a/Neos.ContentGraph.PostgreSQLAdapter/src/Domain/Projection/HypergraphProjection.php b/Neos.ContentGraph.PostgreSQLAdapter/src/Domain/Projection/HypergraphProjection.php index 69ab40e5ce..5961442b9e 100644 --- a/Neos.ContentGraph.PostgreSQLAdapter/src/Domain/Projection/HypergraphProjection.php +++ b/Neos.ContentGraph.PostgreSQLAdapter/src/Domain/Projection/HypergraphProjection.php @@ -40,7 +40,6 @@ use Neos\ContentRepository\Core\Feature\SubtreeTagging\Event\SubtreeWasTagged; use Neos\ContentRepository\Core\Feature\SubtreeTagging\Event\SubtreeWasUntagged; use Neos\ContentRepository\Core\Infrastructure\DbalSchemaDiff; -use Neos\ContentRepository\Core\Infrastructure\ProjectionTransactionTrait; use Neos\ContentRepository\Core\Projection\ContentGraph\ContentGraphProjectionInterface; use Neos\ContentRepository\Core\Projection\ContentGraph\ContentGraphReadModelInterface; use Neos\ContentRepository\Core\Projection\ProjectionStatus; @@ -53,8 +52,6 @@ */ final class HypergraphProjection implements ContentGraphProjectionInterface { - use ProjectionTransactionTrait; - use ContentStreamForking; use NodeCreation; use SubtreeTagging; diff --git a/Neos.ContentRepository.BehavioralTests/Classes/TestSuite/DebugEventProjection.php b/Neos.ContentRepository.BehavioralTests/Classes/TestSuite/DebugEventProjection.php index 959d00db7a..7f71796e96 100644 --- a/Neos.ContentRepository.BehavioralTests/Classes/TestSuite/DebugEventProjection.php +++ b/Neos.ContentRepository.BehavioralTests/Classes/TestSuite/DebugEventProjection.php @@ -12,7 +12,6 @@ use Neos\ContentRepository\Core\EventStore\EventInterface; use Neos\ContentRepository\Core\Infrastructure\DbalSchemaDiff; use Neos\ContentRepository\Core\Infrastructure\DbalSchemaFactory; -use Neos\ContentRepository\Core\Infrastructure\ProjectionTransactionTrait; use Neos\ContentRepository\Core\Projection\ProjectionInterface; use Neos\ContentRepository\Core\Projection\ProjectionStateInterface; use Neos\ContentRepository\Core\Projection\ProjectionStatus; @@ -30,8 +29,6 @@ */ final class DebugEventProjection implements ProjectionInterface { - use ProjectionTransactionTrait; - private DebugEventProjectionState $state; private \Closure|null $saboteur = null; diff --git a/Neos.ContentRepository.BehavioralTests/Tests/Functional/Subscription/AbstractSubscriptionEngineTestCase.php b/Neos.ContentRepository.BehavioralTests/Tests/Functional/Subscription/AbstractSubscriptionEngineTestCase.php index 2a3476bc98..13364c83e6 100644 --- a/Neos.ContentRepository.BehavioralTests/Tests/Functional/Subscription/AbstractSubscriptionEngineTestCase.php +++ b/Neos.ContentRepository.BehavioralTests/Tests/Functional/Subscription/AbstractSubscriptionEngineTestCase.php @@ -77,7 +77,6 @@ public function setUp(): void $this->fakeProjection = $this->getMockBuilder(ProjectionInterface::class)->disableAutoReturnValueGeneration()->getMock(); $this->fakeProjection->method('getState')->willReturn(new class implements ProjectionStateInterface {}); - $this->fakeProjection->expects(self::any())->method('transactional')->willReturnCallback(fn ($fn) => $fn())->willReturnCallback(fn ($fn) => $fn()); FakeProjectionFactory::setProjection( 'default', diff --git a/Neos.ContentRepository.Core/Classes/Infrastructure/ProjectionTransactionTrait.php b/Neos.ContentRepository.Core/Classes/Infrastructure/ProjectionTransactionTrait.php deleted file mode 100644 index 2a6b9d6ebc..0000000000 --- a/Neos.ContentRepository.Core/Classes/Infrastructure/ProjectionTransactionTrait.php +++ /dev/null @@ -1,37 +0,0 @@ -dbal->isTransactionActive() === false) { - /** @phpstan-ignore argument.templateType */ - $this->dbal->transactional($closure); - return; - } - // technically we could leverage nested transactions from dbal, which effectively does the same. - // but that requires us to enable this globally first via setNestTransactionsWithSavepoints also making this explicit is more transparent: - $this->dbal->createSavepoint('PROJECTION'); - try { - $closure(); - } catch (\Throwable $e) { - // roll back the partially applied event on the projection - $this->dbal->rollbackSavepoint('PROJECTION'); - throw $e; - } - $this->dbal->releaseSavepoint('PROJECTION'); - } -} diff --git a/Neos.ContentRepository.Core/Classes/Projection/ProjectionInterface.php b/Neos.ContentRepository.Core/Classes/Projection/ProjectionInterface.php index 19090c17cc..aff5738989 100644 --- a/Neos.ContentRepository.Core/Classes/Projection/ProjectionInterface.php +++ b/Neos.ContentRepository.Core/Classes/Projection/ProjectionInterface.php @@ -30,16 +30,6 @@ public function setUp(): void; */ public function status(): ProjectionStatus; - /** - * Must invoke the closure which will update the catchup hooks and {@see apply}. - * Additionally, to guarantee exactly once delivery and also to behave correct during exceptions (even fatal ones), - * a database transaction should be started, or if a transaction is already active on the same connection save points - * must be used and rolled back on error. - * - * @param-immediately-invoked-callable $closure - */ - public function transactional(\Closure $closure): void; - public function apply(EventInterface $event, EventEnvelope $eventEnvelope): void; /** diff --git a/Neos.ContentRepository.Core/Classes/Subscription/Engine/SubscriptionEngine.php b/Neos.ContentRepository.Core/Classes/Subscription/Engine/SubscriptionEngine.php index 902b4b223c..eb28d4970d 100644 --- a/Neos.ContentRepository.Core/Classes/Subscription/Engine/SubscriptionEngine.php +++ b/Neos.ContentRepository.Core/Classes/Subscription/Engine/SubscriptionEngine.php @@ -337,12 +337,16 @@ private function catchUpSubscriptions(SubscriptionEngineCriteria $criteria, Subs $this->logger?->debug(sprintf('Subscription Engine: Subscription "%s" is farther than the current position (%d >= %d), continue catch up.', $subscription->id->value, $subscription->position->value, $sequenceNumber->value)); continue; } + + $this->subscriptionStore->createSavepoint(); $error = $this->handleEvent($eventEnvelope, $domainEvent, $subscription->id); if ($error !== null) { // ERROR Case: - // 1.) for the leftover events we are not including this failed subscription for catchup + // 1.) roll back the partially applied event on the subscriber + $this->subscriptionStore->rollbackSavepoint(); + // 2.) for the leftover events we are not including this failed subscription for catchup $subscriptionsToCatchup = $subscriptionsToCatchup->without($subscription->id); - // 2.) update the subscription error state on either its unchanged or new position (if some events worked) + // 3.) update the subscription error state on either its unchanged or new position (if some events worked) $this->subscriptionStore->update( $subscription->id, status: SubscriptionStatus::ERROR, @@ -356,6 +360,7 @@ private function catchUpSubscriptions(SubscriptionEngineCriteria $criteria, Subs continue; } // HAPPY Case: + $this->subscriptionStore->releaseSavepoint(); $highestSequenceNumberForSubscriber[$subscription->id->value] = $eventEnvelope->sequenceNumber; } $numberOfProcessedEvents++; diff --git a/Neos.ContentRepository.Core/Classes/Subscription/Store/SubscriptionStoreInterface.php b/Neos.ContentRepository.Core/Classes/Subscription/Store/SubscriptionStoreInterface.php index b7b0540415..85f3107271 100644 --- a/Neos.ContentRepository.Core/Classes/Subscription/Store/SubscriptionStoreInterface.php +++ b/Neos.ContentRepository.Core/Classes/Subscription/Store/SubscriptionStoreInterface.php @@ -35,4 +35,10 @@ public function update( * @return T */ public function transactional(\Closure $closure): mixed; + + public function createSavepoint(): void; + + public function releaseSavepoint(): void; + + public function rollbackSavepoint(): void; } diff --git a/Neos.ContentRepository.Core/Classes/Subscription/Subscriber/ProjectionSubscriber.php b/Neos.ContentRepository.Core/Classes/Subscription/Subscriber/ProjectionSubscriber.php index ee19dbc548..7e15a8fdcb 100644 --- a/Neos.ContentRepository.Core/Classes/Subscription/Subscriber/ProjectionSubscriber.php +++ b/Neos.ContentRepository.Core/Classes/Subscription/Subscriber/ProjectionSubscriber.php @@ -34,11 +34,9 @@ public function onBeforeCatchUp(SubscriptionStatus $subscriptionStatus): void public function handle(EventInterface $event, EventEnvelope $eventEnvelope): void { - $this->projection->transactional(function () use ($event, $eventEnvelope) { - $this->catchUpHook?->onBeforeEvent($event, $eventEnvelope); - $this->projection->apply($event, $eventEnvelope); - $this->catchUpHook?->onAfterEvent($event, $eventEnvelope); - }); + $this->catchUpHook?->onBeforeEvent($event, $eventEnvelope); + $this->projection->apply($event, $eventEnvelope); + $this->catchUpHook?->onAfterEvent($event, $eventEnvelope); } public function onAfterCatchUp(): void diff --git a/Neos.ContentRepositoryRegistry/Classes/Factory/SubscriptionStore/DoctrineSubscriptionStore.php b/Neos.ContentRepositoryRegistry/Classes/Factory/SubscriptionStore/DoctrineSubscriptionStore.php index fe5f038fda..9c7c9e9200 100644 --- a/Neos.ContentRepositoryRegistry/Classes/Factory/SubscriptionStore/DoctrineSubscriptionStore.php +++ b/Neos.ContentRepositoryRegistry/Classes/Factory/SubscriptionStore/DoctrineSubscriptionStore.php @@ -169,4 +169,19 @@ public function transactional(\Closure $closure): mixed { return $this->dbal->transactional($closure); } + + public function createSavepoint(): void + { + $this->dbal->createSavepoint('SUBSCRIBER'); + } + + public function releaseSavepoint(): void + { + $this->dbal->releaseSavepoint('SUBSCRIBER'); + } + + public function rollbackSavepoint(): void + { + $this->dbal->rollbackSavepoint('SUBSCRIBER'); + } } diff --git a/Neos.Neos/Classes/FrontendRouting/Projection/DocumentUriPathProjection.php b/Neos.Neos/Classes/FrontendRouting/Projection/DocumentUriPathProjection.php index 2c2dbe5101..016814020b 100644 --- a/Neos.Neos/Classes/FrontendRouting/Projection/DocumentUriPathProjection.php +++ b/Neos.Neos/Classes/FrontendRouting/Projection/DocumentUriPathProjection.php @@ -25,7 +25,6 @@ use Neos\ContentRepository\Core\Feature\SubtreeTagging\Event\SubtreeWasTagged; use Neos\ContentRepository\Core\Feature\SubtreeTagging\Event\SubtreeWasUntagged; use Neos\ContentRepository\Core\Infrastructure\DbalSchemaDiff; -use Neos\ContentRepository\Core\Infrastructure\ProjectionTransactionTrait; use Neos\ContentRepository\Core\NodeType\NodeTypeManager; use Neos\ContentRepository\Core\NodeType\NodeTypeName; use Neos\ContentRepository\Core\Projection\ProjectionInterface; @@ -41,8 +40,6 @@ */ final class DocumentUriPathProjection implements ProjectionInterface, WithMarkStaleInterface { - use ProjectionTransactionTrait; - public const COLUMN_TYPES_DOCUMENT_URIS = [ 'shortcutTarget' => Types::JSON, ]; diff --git a/Neos.Neos/Classes/PendingChangesProjection/ChangeProjection.php b/Neos.Neos/Classes/PendingChangesProjection/ChangeProjection.php index af0f3e894f..ead06f7b58 100644 --- a/Neos.Neos/Classes/PendingChangesProjection/ChangeProjection.php +++ b/Neos.Neos/Classes/PendingChangesProjection/ChangeProjection.php @@ -39,7 +39,6 @@ use Neos\ContentRepository\Core\Feature\SubtreeTagging\Event\SubtreeWasUntagged; use Neos\ContentRepository\Core\Infrastructure\DbalSchemaDiff; use Neos\ContentRepository\Core\Infrastructure\DbalSchemaFactory; -use Neos\ContentRepository\Core\Infrastructure\ProjectionTransactionTrait; use Neos\ContentRepository\Core\Projection\ProjectionInterface; use Neos\ContentRepository\Core\Projection\ProjectionStatus; use Neos\ContentRepository\Core\SharedModel\Node\NodeAggregateId; @@ -53,8 +52,6 @@ */ class ChangeProjection implements ProjectionInterface { - use ProjectionTransactionTrait; - /** * @var ChangeFinder|null Cache for the ChangeFinder returned by {@see getState()}, * so that always the same instance is returned