diff --git a/src/Helpers/MiddlewarePipeline.php b/src/Helpers/MiddlewarePipeline.php index 06fe94f5..df56675f 100644 --- a/src/Helpers/MiddlewarePipeline.php +++ b/src/Helpers/MiddlewarePipeline.php @@ -37,6 +37,16 @@ public function __construct() $this->fatalPipeline = new Pipeline; } + /** + * Deep-clone internal pipelines so cloned requests do not share pipe state. + */ + public function __clone(): void + { + $this->requestPipeline = clone $this->requestPipeline; + $this->responsePipeline = clone $this->responsePipeline; + $this->fatalPipeline = clone $this->fatalPipeline; + } + /** * Add a middleware before the request is sent * diff --git a/src/Http/Request.php b/src/Http/Request.php index 6f145133..adadf989 100644 --- a/src/Http/Request.php +++ b/src/Http/Request.php @@ -59,6 +59,36 @@ public function getMethod(): Method return $this->method; } + /** + * Ensure cloned requests do not share mutable object state (query, headers, etc.). + * + * Without this, a shallow clone reuses the same {@see \Saloon\Repositories\ArrayStore} + * instances when they were initialized before cloning, which breaks concurrent pools + * (e.g. paginated requests mutating the same query bag). + */ + public function __clone(): void + { + if (isset($this->query)) { + $this->query = clone $this->query; + } + + if (isset($this->headers)) { + $this->headers = clone $this->headers; + } + + if (isset($this->config)) { + $this->config = clone $this->config; + } + + if (isset($this->delay)) { + $this->delay = clone $this->delay; + } + + if (isset($this->middlewarePipeline)) { + $this->middlewarePipeline = clone $this->middlewarePipeline; + } + } + /** * Define the endpoint for the request. */ diff --git a/tests/Unit/RequestCloneTest.php b/tests/Unit/RequestCloneTest.php new file mode 100644 index 00000000..8c6a9648 --- /dev/null +++ b/tests/Unit/RequestCloneTest.php @@ -0,0 +1,86 @@ +query()->add('key', 'value'); + + $a = clone $original; + $b = clone $original; + + $a->query()->add('page', 1); + $b->query()->add('page', 2); + + expect($a->query()->get('page'))->toBe(1); + expect($b->query()->get('page'))->toBe(2); + expect($original->query()->get('page'))->toBeNull(); + expect($original->query()->get('key'))->toBe('value'); +}); + +test('cloning a request after headers config and delay are initialized gives independent stores', function () { + $original = new UserRequest; + $original->headers()->add('X-Test', 'one'); + $original->config()->add('timeout', 10); + $original->delay()->set(5); + + $clone = clone $original; + + $clone->headers()->add('X-Test', 'two'); + $clone->config()->add('timeout', 20); + $clone->delay()->set(15); + + expect($original->headers()->get('X-Test'))->toBe('one'); + expect($clone->headers()->get('X-Test'))->toBe('two'); + expect($original->config()->get('timeout'))->toBe(10); + expect($clone->config()->get('timeout'))->toBe(20); + expect($original->delay()->get())->toBe(5); + expect($clone->delay()->get())->toBe(15); +}); + +test('cloning a request after middleware is initialized gives independent pipelines', function () { + $original = new UserRequest; + $original->middleware()->onResponse(static fn (Response $response): Response => $response); + + $clone = clone $original; + + expect($original->middleware())->not->toBe($clone->middleware()); +}); + +test('concurrent pool sends with cloned requests do not share query mutation', function () { + $sequence = []; + for ($i = 0; $i < 10; $i++) { + $sequence[] = MockResponse::make(['ok' => true]); + } + + $connector = new TestConnector; + $connector->withMockClient(new MockClient($sequence)); + + $base = new UserRequest; + $base->query()->add('key', 'value'); + + $requests = []; + for ($i = 1; $i <= 10; $i++) { + $r = clone $base; + $r->query()->add('page', $i); + $requests[] = $r; + } + + $pagesSeen = []; + + $pool = $connector->pool($requests, 5); + $pool->withResponseHandler(function (Response $response) use (&$pagesSeen): void { + $pagesSeen[] = (int) $response->getRequest()->query()->get('page'); + }); + + $pool->send()->wait(); + + expect($pagesSeen)->toHaveCount(10); + expect(array_unique($pagesSeen))->toHaveCount(10); +});