-
-
Notifications
You must be signed in to change notification settings - Fork 144
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Expose DuplexStreamInterface for connections that requires Upgrades #382
Changes from 14 commits
f73b33f
7cc5c5a
662df47
ccca59c
6004504
3585dca
9a23b03
9a02480
88d862f
532755a
65f563c
b3264a5
7f3890f
26b3b9a
17b503a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ | |
use React\Http\Io\Sender; | ||
use React\Http\Io\Transaction; | ||
use React\Promise\PromiseInterface; | ||
use React\Socket\ConnectionInterface; | ||
use React\Socket\ConnectorInterface; | ||
use React\Stream\ReadableStreamInterface; | ||
use InvalidArgumentException; | ||
|
@@ -707,7 +708,7 @@ public function withResponseBuffer($maximumSize) | |
* @see self::withFollowRedirects() | ||
* @see self::withRejectErrorResponse() | ||
*/ | ||
private function withOptions(array $options) | ||
public function withOptions(array $options) | ||
{ | ||
$browser = clone $this; | ||
$browser->transaction = $this->transaction->withOptions($options); | ||
|
@@ -720,7 +721,7 @@ private function withOptions(array $options) | |
* @param string $url | ||
* @param array $headers | ||
* @param string|ReadableStreamInterface $body | ||
* @return PromiseInterface<ResponseInterface,Exception> | ||
* @return PromiseInterface<ResponseInterface,Exception, ConnectionInterface> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure what this is supposed to mean? The promise either fulfills with a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added it in the case, where an |
||
*/ | ||
private function requestMayBeStreaming($method, $url, array $headers = array(), $body = '') | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -89,6 +89,14 @@ function (ConnectionInterface $stream) use ($requestData, &$streamRef, &$stateRe | |
}); | ||
} | ||
|
||
protected function responseIsAnUpgradeResponse($response) | ||
{ | ||
return | ||
$response->hasHeader('Connection') && | ||
(in_array('upgrade', array_map('strtolower', $response->getHeader('Connection')))) && | ||
(int) $response->getStatusCode() === 101; | ||
} | ||
|
||
public function write($data) | ||
{ | ||
if (!$this->isWritable()) { | ||
|
@@ -138,7 +146,16 @@ public function handleData($data) | |
// buffer until double CRLF (or double LF for compatibility with legacy servers) | ||
if (false !== strpos($this->buffer, "\r\n\r\n") || false !== strpos($this->buffer, "\n\n")) { | ||
try { | ||
list($response, $bodyChunk) = $this->parseResponse($this->buffer); | ||
$psrResponse = gPsr\parse_response($this->buffer); | ||
|
||
if ($this->responseIsAnUpgradeResponse($psrResponse)) { | ||
$this->stream->removeListener('data', array($this, 'handleData')); | ||
|
||
$this->emit('upgrade', array($this->stream, $psrResponse, $this)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure I like exposing the Do we really need to expose the response and request objects? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if this is the right I actually read a piece of Node docs to see how Sure, If there's a better you think this could be or should be done, I'll be willing to make it work. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure about the whole I like how this is exposed on the server side via https://github.com/reactphp/http#streaming-outgoing-response, perhaps we can find a similar solution here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually followed a Node piece I read here as I also mentioned above. I will check out the sample you sent, I kind of like it, knowing that a |
||
return; | ||
} | ||
|
||
list($response, $bodyChunk) = $this->parseResponse($psrResponse); | ||
} catch (\InvalidArgumentException $exception) { | ||
$this->emit('error', array($exception)); | ||
} | ||
|
@@ -222,9 +239,8 @@ public function close() | |
$this->removeAllListeners(); | ||
} | ||
|
||
protected function parseResponse($data) | ||
protected function parseResponse($psrResponse) | ||
{ | ||
$psrResponse = gPsr\parse_response($data); | ||
$headers = array_map(function($val) { | ||
if (1 === count($val)) { | ||
$val = $val[0]; | ||
|
@@ -292,4 +308,9 @@ public function getResponseFactory() | |
|
||
return $factory; | ||
} | ||
|
||
public function getRequestData() | ||
{ | ||
return $this->requestData; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
<?php | ||
|
||
namespace React\Http\Client; | ||
|
||
use Psr\Http\Message\RequestInterface; | ||
use Psr\Http\Message\ResponseInterface; | ||
use React\Stream\DuplexStreamInterface; | ||
|
||
class UpgradedResponse | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This adds a new class as part of our public API in the internal |
||
{ | ||
/** | ||
* @var DuplexStreamInterface | ||
*/ | ||
private $connection; | ||
|
||
/** | ||
* @var ResponseInterface | ||
*/ | ||
private $response; | ||
|
||
/** | ||
* @var RequestInterface | ||
*/ | ||
private $request; | ||
|
||
public function __construct(DuplexStreamInterface $connection, ResponseInterface $response, RequestInterface $request) | ||
{ | ||
$this->connection = $connection; | ||
$this->response = $response; | ||
$this->request = $request; | ||
} | ||
|
||
/** | ||
* @return DuplexStreamInterface | ||
*/ | ||
public function getConnection() | ||
{ | ||
return $this->connection; | ||
} | ||
|
||
/** | ||
* @return ResponseInterface | ||
*/ | ||
public function getResponse() | ||
{ | ||
return $this->response; | ||
} | ||
|
||
/** | ||
* @return RequestInterface | ||
*/ | ||
public function getRequest() | ||
{ | ||
return $this->request; | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: missing EOL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I like exposing the
withOptions()
method again. It has been deprecated with clue/reactphp-buzz#172 not too long ago.Do we really need this method? It's my understanding we could just rely on the
Connection: upgrade
and/orUpgrade: …
request header(s) being present?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@clue I didn't know the method was deprecated.
But my thought behind it was to let the consumer using
Browser
to be able to choose to watch out for the upgrade event or not. As by default,Browser
will not. This means that, if an upgrade did happen, I need to configure browser to capture it like$browser->withOptions(['upgrade' => true])
(default isfalse
inTransaction.php
).This, however, won't be necessary if the default mode should be to capture the upgrade. That was the reason I made the method public (also seeing it was public in
clue/reactphp-buzz
)