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..e4d8c88 100644 --- a/tests/DataLoadTestCase.php +++ b/tests/DataLoadTestCase.php @@ -541,6 +541,57 @@ 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); + + $this->assertSame($throwable, DataLoader::await($promise1, false)); + $this->assertSame($throwable, DataLoader::await($promise2, false)); + + $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); + }); + + $this->assertInstanceOf(\Exception::class, DataLoader::await($loader->load(1), false)); + $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 */