From 09ddbff7abe861329af6ae9e803f68eb38aa2014 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Tue, 31 Jul 2018 00:53:08 +0200 Subject: [PATCH 1/2] Add tests to reproduce redirection cancellation issues --- tests/BrowserTest.php | 28 ++++++++ tests/Io/TransactionTest.php | 125 ++++++++++++++++++++++++++++++++++- 2 files changed, 152 insertions(+), 1 deletion(-) diff --git a/tests/BrowserTest.php b/tests/BrowserTest.php index a9d0e04..6732d10 100644 --- a/tests/BrowserTest.php +++ b/tests/BrowserTest.php @@ -162,4 +162,32 @@ public function testWithBaseUriNotAbsoluteFails() { $this->browser->withBase('hello'); } + + public function testCancelGetRequestShouldCancelUnderlyingSocketConnection() + { + $pending = new Promise(function () { }, $this->expectCallableOnce()); + + $connector = $this->getMockBuilder('React\Socket\ConnectorInterface')->getMock(); + $connector->expects($this->once())->method('connect')->with('example.com:80')->willReturn($pending); + + $this->browser = new Browser($this->loop, $connector); + + $promise = $this->browser->get('http://example.com/'); + $promise->cancel(); + } + + protected function expectCallableOnce() + { + $mock = $this->createCallableMock(); + $mock + ->expects($this->once()) + ->method('__invoke'); + + return $mock; + } + + protected function createCallableMock() + { + return $this->getMockBuilder('stdClass')->setMethods(array('__invoke'))->getMock(); + } } diff --git a/tests/Io/TransactionTest.php b/tests/Io/TransactionTest.php index fe1ea6f..745ff2b 100644 --- a/tests/Io/TransactionTest.php +++ b/tests/Io/TransactionTest.php @@ -10,6 +10,7 @@ use Clue\React\Block; use React\EventLoop\Factory; use React\Stream\ThroughStream; +use React\Promise\Deferred; class TransactionTest extends TestCase { @@ -84,7 +85,7 @@ public function testCancelBufferingResponseWillCloseStreamAndReject() $promise = $transaction->send(); $promise->cancel(); - Block\await($promise, $loop); + Block\await($promise, $loop, 0.001); } public function testReceivingStreamingBodyWillResolveWithStreamingResponseIfStreamingIsEnabled() @@ -252,6 +253,113 @@ public function testSomeRequestHeadersShouldBeRemovedWhenRedirecting() $transaction->send(); } + public function testCancelTransactionWillCancelRequest() + { + $messageFactory = new MessageFactory(); + + $request = $messageFactory->request('GET', 'http://example.com'); + $sender = $this->makeSenderMock(); + + $pending = new \React\Promise\Promise(function () { }, $this->expectCallableOnce()); + + // mock sender to return pending promise which should be cancelled when cancelling result + $sender->expects($this->once())->method('send')->willReturn($pending); + + $transaction = new Transaction($request, $sender, array(), $messageFactory); + $promise = $transaction->send(); + + $promise->cancel(); + } + + public function testCancelTransactionWillCancelRedirectedRequest() + { + $messageFactory = new MessageFactory(); + + $request = $messageFactory->request('GET', 'http://example.com'); + $sender = $this->makeSenderMock(); + + // mock sender to resolve promise with the given $redirectResponse in + $redirectResponse = $messageFactory->response(1.0, 301, null, array('Location' => 'http://example.com/new')); + $sender->expects($this->at(0))->method('send')->willReturn(Promise\resolve($redirectResponse)); + + $pending = new \React\Promise\Promise(function () { }, $this->expectCallableOnce()); + + // mock sender to return pending promise which should be cancelled when cancelling result + $sender->expects($this->at(1))->method('send')->willReturn($pending); + + $transaction = new Transaction($request, $sender, array(), $messageFactory); + $promise = $transaction->send(); + + $promise->cancel(); + } + + public function testCancelTransactionWillCancelRedirectedRequestAgain() + { + $messageFactory = new MessageFactory(); + + $request = $messageFactory->request('GET', 'http://example.com'); + $sender = $this->makeSenderMock(); + + // mock sender to resolve promise with the given $redirectResponse in + $first = new Deferred(); + $sender->expects($this->at(0))->method('send')->willReturn($first->promise()); + + $second = new \React\Promise\Promise(function () { }, $this->expectCallableOnce()); + + // mock sender to return pending promise which should be cancelled when cancelling result + $sender->expects($this->at(1))->method('send')->willReturn($second); + + $transaction = new Transaction($request, $sender, array(), $messageFactory); + $promise = $transaction->send(); + + // mock sender to resolve promise with the given $redirectResponse in + $first->resolve($messageFactory->response(1.0, 301, null, array('Location' => 'http://example.com/new'))); + + $promise->cancel(); + } + + public function testCancelTransactionWillCloseBufferingStream() + { + $messageFactory = new MessageFactory(); + + $request = $messageFactory->request('GET', 'http://example.com'); + $sender = $this->makeSenderMock(); + + $body = new ThroughStream(); + $body->on('close', $this->expectCallableOnce()); + + // mock sender to resolve promise with the given $redirectResponse in + $redirectResponse = $messageFactory->response(1.0, 301, null, array('Location' => 'http://example.com/new'), $body); + $sender->expects($this->once())->method('send')->willReturn(Promise\resolve($redirectResponse)); + + $transaction = new Transaction($request, $sender, array(), $messageFactory); + $promise = $transaction->send(); + + $promise->cancel(); + } + + public function testCancelTransactionWillCloseBufferingStreamAgain() + { + $messageFactory = new MessageFactory(); + + $request = $messageFactory->request('GET', 'http://example.com'); + $sender = $this->makeSenderMock(); + + $first = new Deferred(); + $sender->expects($this->once())->method('send')->willReturn($first->promise()); + + $transaction = new Transaction($request, $sender, array(), $messageFactory); + $promise = $transaction->send(); + + $body = new ThroughStream(); + $body->on('close', $this->expectCallableOnce()); + + // mock sender to resolve promise with the given $redirectResponse in + $first->resolve($messageFactory->response(1.0, 301, null, array('Location' => 'http://example.com/new'), $body)); + + $promise->cancel(); + } + /** * @return MockObject */ @@ -259,4 +367,19 @@ private function makeSenderMock() { return $this->getMockBuilder('Clue\React\Buzz\Io\Sender')->disableOriginalConstructor()->getMock(); } + + protected function expectCallableOnce() + { + $mock = $this->createCallableMock(); + $mock + ->expects($this->once()) + ->method('__invoke'); + + return $mock; + } + + protected function createCallableMock() + { + return $this->getMockBuilder('stdClass')->setMethods(array('__invoke'))->getMock(); + } } From cf5b1e141ba041224a0b0fe9022d7b5e318e5149 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Mon, 1 Oct 2018 15:08:46 +0200 Subject: [PATCH 2/2] Support cancellation forwarding and cancelling redirected requests --- composer.json | 2 +- examples/03-any.php | 32 ++++++++++++++++++++++ src/Io/Sender.php | 4 +-- src/Io/Transaction.php | 48 ++++++++++++++++++++++----------- tests/FunctionalBrowserTest.php | 16 +++++++++++ tests/Io/TransactionTest.php | 21 +++++++++++++++ 6 files changed, 105 insertions(+), 18 deletions(-) create mode 100644 examples/03-any.php diff --git a/composer.json b/composer.json index 682eb7f..e3f00eb 100644 --- a/composer.json +++ b/composer.json @@ -20,7 +20,7 @@ "react/http-client": "^0.5.8", "react/promise": "^2.2.1 || ^1.2.1", "react/promise-stream": "^1.0 || ^0.1.1", - "react/socket": "^1.0 || ^0.8.4", + "react/socket": "^1.1", "react/stream": "^1.0 || ^0.7", "ringcentral/psr7": "^1.2" }, diff --git a/examples/03-any.php b/examples/03-any.php new file mode 100644 index 0000000..426a0e2 --- /dev/null +++ b/examples/03-any.php @@ -0,0 +1,32 @@ +head('http://www.github.com/clue/http-react'), + $client->get('https://httpbin.org/'), + $client->get('https://google.com'), + $client->get('http://www.lueck.tv/psocksd'), + $client->get('http://www.httpbin.org/absolute-redirect/5') +); + +React\Promise\any($promises)->then(function (ResponseInterface $response) use ($promises) { + // first response arrived => cancel all other pending requests + foreach ($promises as $promise) { + $promise->cancel(); + } + + var_dump($response->getHeaders()); + echo PHP_EOL . $response->getBody(); +}); + +$loop->run(); diff --git a/src/Io/Sender.php b/src/Io/Sender.php index 03d43cc..774c4ab 100644 --- a/src/Io/Sender.php +++ b/src/Io/Sender.php @@ -105,8 +105,8 @@ public function send(RequestInterface $request, MessageFactory $messageFactory) $requestStream = $this->http->request($request->getMethod(), (string)$uri, $headers, $request->getProtocolVersion()); $deferred = new Deferred(function ($_, $reject) use ($requestStream) { - // close request stream if request is canceled - $reject(new \RuntimeException('Request canceled')); + // close request stream if request is cancelled + $reject(new \RuntimeException('Request cancelled')); $requestStream->close(); }); diff --git a/src/Io/Transaction.php b/src/Io/Transaction.php index 6eb5849..cad55db 100644 --- a/src/Io/Transaction.php +++ b/src/Io/Transaction.php @@ -7,11 +7,9 @@ use Psr\Http\Message\RequestInterface; use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\UriInterface; -use React\Promise; +use React\Promise\Deferred; use React\Promise\PromiseInterface; -use React\Promise\Stream; use React\Stream\ReadableStreamInterface; -use Exception; /** * @internal @@ -50,10 +48,22 @@ public function __construct(RequestInterface $request, Sender $sender, array $op public function send() { - return $this->next($this->request); + $deferred = new Deferred(function () use (&$deferred) { + if (isset($deferred->pending)) { + $deferred->pending->cancel(); + unset($deferred->pending); + } + }); + + $this->next($this->request, $deferred)->then( + array($deferred, 'resolve'), + array($deferred, 'reject') + ); + + return $deferred->promise(); } - private function next(RequestInterface $request) + private function next(RequestInterface $request, Deferred $deferred) { $this->progress('request', array($request)); @@ -63,12 +73,16 @@ private function next(RequestInterface $request) $promise = $this->sender->send($request, $this->messageFactory); if (!$this->streaming) { - $promise = $promise->then(array($that, 'bufferResponse')); + $promise = $promise->then(function ($response) use ($deferred, $that) { + return $that->bufferResponse($response, $deferred); + }); } + $deferred->pending = $promise; + return $promise->then( - function (ResponseInterface $response) use ($request, $that) { - return $that->onResponse($response, $request); + function (ResponseInterface $response) use ($request, $that, $deferred) { + return $that->onResponse($response, $request, $deferred); } ); } @@ -78,18 +92,18 @@ function (ResponseInterface $response) use ($request, $that) { * @param ResponseInterface $response * @return PromiseInterface Promise */ - public function bufferResponse(ResponseInterface $response) + public function bufferResponse(ResponseInterface $response, $deferred) { $stream = $response->getBody(); // body is not streaming => already buffered if (!$stream instanceof ReadableStreamInterface) { - return Promise\resolve($response); + return \React\Promise\resolve($response); } // buffer stream and resolve with buffered body $messageFactory = $this->messageFactory; - return Stream\buffer($stream)->then( + $promise = \React\Promise\Stream\buffer($stream)->then( function ($body) use ($response, $messageFactory) { return $response->withBody($messageFactory->body($body)); }, @@ -100,6 +114,10 @@ function ($e) use ($stream) { throw $e; } ); + + $deferred->pending = $promise; + + return $promise; } /** @@ -109,12 +127,12 @@ function ($e) use ($stream) { * @throws ResponseException * @return ResponseInterface|PromiseInterface */ - public function onResponse(ResponseInterface $response, RequestInterface $request) + public function onResponse(ResponseInterface $response, RequestInterface $request, $deferred) { $this->progress('response', array($response, $request)); if ($this->followRedirects && ($response->getStatusCode() >= 300 && $response->getStatusCode() < 400)) { - return $this->onResponseRedirect($response, $request); + return $this->onResponseRedirect($response, $request, $deferred); } // only status codes 200-399 are considered to be valid, reject otherwise @@ -132,7 +150,7 @@ public function onResponse(ResponseInterface $response, RequestInterface $reques * @return PromiseInterface * @throws \RuntimeException */ - private function onResponseRedirect(ResponseInterface $response, RequestInterface $request) + private function onResponseRedirect(ResponseInterface $response, RequestInterface $request, $deferred) { // resolve location relative to last request URI $location = $this->messageFactory->uriRelative($request->getUri(), $response->getHeaderLine('Location')); @@ -144,7 +162,7 @@ private function onResponseRedirect(ResponseInterface $response, RequestInterfac throw new \RuntimeException('Maximum number of redirects (' . $this->maxRedirects . ') exceeded'); } - return $this->next($request); + return $this->next($request, $deferred); } /** diff --git a/tests/FunctionalBrowserTest.php b/tests/FunctionalBrowserTest.php index 4195974..68670f0 100644 --- a/tests/FunctionalBrowserTest.php +++ b/tests/FunctionalBrowserTest.php @@ -98,6 +98,22 @@ public function testRedirectFromPageWithInvalidAuthToPageWithCorrectAuthenticati Block\await($this->browser->get($base . 'redirect-to?url=' . urlencode($target)), $this->loop); } + /** + * @expectedException RuntimeException + * @expectedExceptionMessage Request cancelled + * @group online + */ + public function testCancelRedirectedRequestShouldReject() + { + $promise = $this->browser->get($this->base . 'redirect-to?url=delay%2F10'); + + $this->loop->addTimer(0.1, function () use ($promise) { + $promise->cancel(); + }); + + Block\await($promise, $this->loop); + } + /** * @group online * @doesNotPerformAssertions diff --git a/tests/Io/TransactionTest.php b/tests/Io/TransactionTest.php index 745ff2b..d6b54c8 100644 --- a/tests/Io/TransactionTest.php +++ b/tests/Io/TransactionTest.php @@ -356,6 +356,27 @@ public function testCancelTransactionWillCloseBufferingStreamAgain() // mock sender to resolve promise with the given $redirectResponse in $first->resolve($messageFactory->response(1.0, 301, null, array('Location' => 'http://example.com/new'), $body)); + $promise->cancel(); + } + + public function testCancelTransactionShouldCancelSendingPromise() + { + $messageFactory = new MessageFactory(); + + $request = $messageFactory->request('GET', 'http://example.com'); + $sender = $this->makeSenderMock(); + + // mock sender to resolve promise with the given $redirectResponse in + $redirectResponse = $messageFactory->response(1.0, 301, null, array('Location' => 'http://example.com/new')); + $sender->expects($this->at(0))->method('send')->willReturn(Promise\resolve($redirectResponse)); + + $pending = new \React\Promise\Promise(function () { }, $this->expectCallableOnce()); + + // mock sender to return pending promise which should be cancelled when cancelling result + $sender->expects($this->at(1))->method('send')->willReturn($pending); + + $transaction = new Transaction($request, $sender, array(), $messageFactory); + $promise = $transaction->send(); $promise->cancel(); }