From b7a0500b5c2835af89f142c89b360529483d4466 Mon Sep 17 00:00:00 2001 From: Simon Podlipsky Date: Mon, 17 Aug 2026 16:21:07 +0200 Subject: [PATCH 1/2] fix: reject synchronous batch callback failures --- src/DataLoader.php | 17 +++++++--- tests/DataLoadTestCase.php | 67 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 5 deletions(-) diff --git a/src/DataLoader.php b/src/DataLoader.php index a016c68..68b7831 100644 --- a/src/DataLoader.php +++ b/src/DataLoader.php @@ -91,6 +91,11 @@ function () { 'promise' => $promise, ]; + // If caching, cache this promise before it may be dispatched. + if ($shouldCache) { + $this->promiseCache->set($cacheKey, $promise); + } + // Determine if a dispatch of this queue should be scheduled. // A single dispatch should be scheduled per queue at the time when the // queue changes from "empty" to "full". @@ -100,10 +105,6 @@ function () { $this->dispatchQueue(); } } - // If caching, cache this promise. - if ($shouldCache) { - $this->promiseCache->set($cacheKey, $promise); - } return $promise; } @@ -331,7 +332,13 @@ private function dispatchQueueBatch(array $queue) // Call the provided batchLoadFn for this loader with the loader queue's keys. $batchLoadFn = $this->batchLoadFn; - $batchPromise = $batchLoadFn($keys); + try { + $batchPromise = $batchLoadFn($keys); + } catch (\Throwable $error) { + $this->failedDispatch($queue, $error); + + return; + } // Assert the expected response from batchLoadFn if (!$batchPromise || !is_callable([$batchPromise, 'then'])) { diff --git a/tests/DataLoadTestCase.php b/tests/DataLoadTestCase.php index 95de95c..2d3029e 100644 --- a/tests/DataLoadTestCase.php +++ b/tests/DataLoadTestCase.php @@ -541,6 +541,73 @@ public function testPropagatesErrorFromFailedBatchToAllLoads() $this->assertEquals([[1, 2]], $loadCalls->getArrayCopy()); } + /** + * @dataProvider provideSynchronousBatchThrowable + */ + public function testPropagatesSynchronousBatchThrowableToAllLoadsAndAllowsRetry(\Throwable $throwable) + { + $attempt = 0; + list($loader, $loadCalls) = self::idLoader(null, function ($keys) use (&$attempt, $throwable) { + if (0 === $attempt++) { + throw $throwable; + } + + return self::$promiseAdapter->createFulfilled($keys); + }); + + $promise1 = $loader->load(1); + $promise2 = $loader->load(2); + + $rejectedReason1 = null; + $promise1->then(null, function ($reason) use (&$rejectedReason1) { + $rejectedReason1 = $reason; + }); + $rejectedReason2 = null; + $promise2->then(null, function ($reason) use (&$rejectedReason2) { + $rejectedReason2 = $reason; + }); + + DataLoader::await(); + + $this->assertSame($throwable, $rejectedReason1); + $this->assertSame($throwable, $rejectedReason2); + + $this->assertEquals( + [1, 2], + DataLoader::await(self::$promiseAdapter->createAll([$loader->load(1), $loader->load(2)])) + ); + $this->assertEquals([[1, 2], [1, 2]], $loadCalls->getArrayCopy()); + } + + public function testDoesNotCacheSynchronousBatchFailureWhenBatchingIsDisabled() + { + $attempt = 0; + list($loader, $loadCalls) = self::idLoader(new Option(['batch' => false]), function ($keys) use (&$attempt) { + if (0 === $attempt++) { + throw new \Exception('Synchronous batch exception'); + } + + return self::$promiseAdapter->createFulfilled($keys); + }); + + $rejectedReason = null; + $loader->load(1)->then(null, function ($reason) use (&$rejectedReason) { + $rejectedReason = $reason; + }); + + $this->assertInstanceOf(\Exception::class, $rejectedReason); + $this->assertEquals(1, DataLoader::await($loader->load(1))); + $this->assertEquals([[1], [1]], $loadCalls->getArrayCopy()); + } + + public static function provideSynchronousBatchThrowable() + { + return [ + 'exception' => [new \Exception('Synchronous batch exception')], + 'error' => [new \Error('Synchronous batch error')], + ]; + } + /** * @group accepts-any-kind-of-key */ From 795227ff1421737576b723a2947787f5e7920cf2 Mon Sep 17 00:00:00 2001 From: Simon Podlipsky Date: Tue, 18 Aug 2026 09:34:42 +0200 Subject: [PATCH 2/2] test: await synchronous batch rejections --- tests/DataLoadTestCase.php | 22 +++------------------- 1 file changed, 3 insertions(+), 19 deletions(-) diff --git a/tests/DataLoadTestCase.php b/tests/DataLoadTestCase.php index 2d3029e..e4d8c88 100644 --- a/tests/DataLoadTestCase.php +++ b/tests/DataLoadTestCase.php @@ -558,19 +558,8 @@ public function testPropagatesSynchronousBatchThrowableToAllLoadsAndAllowsRetry( $promise1 = $loader->load(1); $promise2 = $loader->load(2); - $rejectedReason1 = null; - $promise1->then(null, function ($reason) use (&$rejectedReason1) { - $rejectedReason1 = $reason; - }); - $rejectedReason2 = null; - $promise2->then(null, function ($reason) use (&$rejectedReason2) { - $rejectedReason2 = $reason; - }); - - DataLoader::await(); - - $this->assertSame($throwable, $rejectedReason1); - $this->assertSame($throwable, $rejectedReason2); + $this->assertSame($throwable, DataLoader::await($promise1, false)); + $this->assertSame($throwable, DataLoader::await($promise2, false)); $this->assertEquals( [1, 2], @@ -590,12 +579,7 @@ public function testDoesNotCacheSynchronousBatchFailureWhenBatchingIsDisabled() return self::$promiseAdapter->createFulfilled($keys); }); - $rejectedReason = null; - $loader->load(1)->then(null, function ($reason) use (&$rejectedReason) { - $rejectedReason = $reason; - }); - - $this->assertInstanceOf(\Exception::class, $rejectedReason); + $this->assertInstanceOf(\Exception::class, DataLoader::await($loader->load(1), false)); $this->assertEquals(1, DataLoader::await($loader->load(1))); $this->assertEquals([[1], [1]], $loadCalls->getArrayCopy()); }