[WIP] Expose DuplexStreamInterface for connections that requires Upgrades #382

bosunski · 2020-07-27T20:51:36Z

Based on the Idea of #376 by @clue

Emits an upgrade event when:

Connection header is present in the response headers and values has upgrade
Response status code is 101

Event emitted exposes a React\Stream\DuplexStreamInterface, a Psr\Http\Message\ResponseInterface and React\Http\Client\Request.

Summary of changes

Adds custom header parser to parse response data before data is passed down to the main data event parser.
Adds detection of Upgraded responses
Updated React\Http\Io\Sender and React\Http\Io\Transaction to be configurable for Upgrade connections

Example Usage:

$request = $client->request("GET", 'https://echo.websocket.org:443', ['Connection' => 'Upgrade', 'Upgrade' => 'websocket']);

$request->on('upgrade', function (DuplexStreamInterface $connection, $response, $request) {
    echo "Request Upgraded";
});

Websocket Client Usage:

$promise = $browser->withOptions(['upgrade' => true])->get('https://echo.websocket.org:443', ['Connection' => 'Upgrade', 'Upgrade' => 'websocket']);

$response->then(function (UpgradedResponse $response) use ($loop) {
    $ws = new \Ratchet\ClientWebSocket($response->getConnection(), $response->getResponse(), $response->getRequest());
});

Example Usage in clue/reactphp-docker

$this->browser->withOptions(array('streaming' => true, 'upgrade' => true))->post(
            $this->uri->expand(
                '/exec/{exec}/start',
                array(
                    'exec' => $exec
                )
            ),
            array(
                'Content-Type' => 'application/json',
                'Connection' => 'Upgrade',
                'Upgrade' => 'tcp',
            ),
            $this->json(array(
                'Tty' => !!$tty
            ))
  );

bosunski · 2020-07-27T21:04:05Z

Still working on some failing tests, you can help with review while I fix the tests.
Thanks!
cc @clue @WyriHaximus

…sponse

clue

@bosunski Thanks for looking into this, would love to get this feature in!

It looks like this is still a work in progress, but I like how this doesn't seem to affect a lot of code at the moment. I've added some minor remarks below.

Let us know if there's anything we can help with. Keep it up! 💪

clue · 2020-08-11T21:56:32Z

src/Browser.php

@@ -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>


Not sure what this is supposed to mean? The promise either fulfills with a ResponseInterface or rejects with an Exception.

I added it in the case, where an upgrade did happen, the promise will resolve with a ConnectionInterface rather than an Exception or a ResponseInterface. Still not sure if this is the best way to express that.

clue · 2020-08-11T21:57:35Z

src/Browser.php

@@ -707,7 +708,7 @@ public function withResponseBuffer($maximumSize)
     * @see self::withFollowRedirects()
     * @see self::withRejectErrorResponse()
     */
-    private function withOptions(array $options)
+    public function withOptions(array $options)


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/or Upgrade: … request header(s) being present?

@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 is false in Transaction.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)

clue · 2020-08-11T21:58:56Z

src/Client/Request.php

+                if ($this->responseIsAnUpgradeResponse($psrResponse)) {
+                    $this->stream->removeListener('data', array($this, 'handleData'));
+
+                    $this->emit('upgrade', array($this->stream, $psrResponse, $this));


Not sure I like exposing the React\Http\Client\Request which is currently marked as @internal only. There's ongoing effort to remove all classes from this namespace as there's some duplication with the Io namespace.

Do we really need to expose the response and request objects?

Not sure if this is the right Request object to expose, but it's kind of a good idea to expose a request object because there could be cases where you want to verify some things about the request after upgrade and then close the connection if that verification fails. That's my thought about it. But sure, I might be wrong here.

I actually read a piece of Node docs to see how upgrade is being handled by the consumer side. I followed that in adding the upgrade event also. Here is a sample piece of how node handles the upgrade.

Sure, If there's a better you think this could be or should be done, I'll be willing to make it work.

clue · 2020-08-11T22:03:17Z

src/Client/Request.php

+                if ($this->responseIsAnUpgradeResponse($psrResponse)) {
+                    $this->stream->removeListener('data', array($this, 'handleData'));
+
+                    $this->emit('upgrade', array($this->stream, $psrResponse, $this));


Not sure about the whole upgrade event. How does this interfere with the existing request/response semantics?

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?

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 ThroughStream also implements DuplexStreamInterface.

bosunski · 2022-08-31T14:28:04Z

Hello @clue , are there anything that can be done to move this PR forward since this is still a desired feature?

clue · 2022-09-19T16:16:20Z

src/Client/UpgradedResponse.php

+    {
+        return $this->request;
+    }
+}


nit: missing EOL

clue · 2022-09-19T16:17:07Z

src/Client/UpgradedResponse.php

+use Psr\Http\Message\ResponseInterface;
+use React\Stream\DuplexStreamInterface;
+
+class UpgradedResponse


This adds a new class as part of our public API in the internal React\Http\Client namespace.

clue · 2022-09-19T16:18:36Z

src/Browser.php

@@ -770,7 +771,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>


What is this supposed to do? The PromiseInterface does not currently nor the foreseeable future have a third template type.

clue · 2022-09-19T16:22:28Z

Hello @clue , are there anything that can be done to move this PR forward since this is still a desired feature?

@bosunski Thank you for the friendly reminder! I agree this feature would be nice to have, but in all honestly I don't see the currently suggested API as a viable alternative. We're moving towards a more type-safe future (see also https://github.com/orgs/reactphp/discussions/472) and this PR currently seems to add APIs that can not be covered by the existing type system. I think we're on the same page that the underlying feature request is still interesting, but I wonder what a decent API could look like?

bosunski · 2022-09-22T08:10:08Z

Thanks for your response @clue

This PR is already 2 years old 😅 and I also agree that this PR needs a reimplementation to be consistent with the changes that have happened within ReactPHP itself and also to align with the current goals.

I'm still very interested in getting this out of the way.

Maybe we can start by talking about how the upgrade API will be used from a developers' point of view.

clue · 2022-10-04T15:47:30Z

@bosunski Agreed, let's continue the discussion in the feature ticket #376 and close this PR. May I ask you to post a quick gist in that ticket so others can follow what's been decided so far? Thank you!

bosunski added 9 commits July 26, 2020 12:13

Begin Writable implementation

f73b33f

Add upgrade listener and provide option for upgrade in Transaction

7cc5c5a

Add upgrade listener and provide option for upgrade in Transaction

662df47

Reverted Scope

ccca59c

Fix conflicts

6004504

Configures sender to upgrade or not

3585dca

Add check for upgrade on response instead of request

9a23b03

Remove isUpgradeRequest

9a02480

Remove requestUpgrade method

88d862f

WyriHaximus requested review from clue, WyriHaximus and jsor July 27, 2020 20:56

bosunski mentioned this pull request Jul 27, 2020

Adds Support for TCP Hijack in execStartStream clue/reactphp-docker#63

Open

WyriHaximus added the new feature label Jul 27, 2020

bosunski added 5 commits July 27, 2020 22:08

Replaced ConnectionInteface With DuplexStreamInterface In upgraded re…

532755a

…sponse

Replaced ConnectionInteface With DuplexStreamInterface In Sender

65f563c

Start fixing tests

b3264a5

Refactored some parts so tests can pass

7f3890f

Remove comment

26b3b9a

bosunski changed the title ~~Exposes an upgraded DuplexStreamInterface for connections that requires Upgrades~~ Exposes a DuplexStreamInterface for connections that requires Upgrades Jul 28, 2020

clue changed the title ~~Exposes a DuplexStreamInterface for connections that requires Upgrades~~ [WIP] Expose DuplexStreamInterface for connections that requires Upgrades Aug 11, 2020

clue reviewed Aug 11, 2020

View reviewed changes

clue added the help wanted label Aug 11, 2020

Resolve conflicts

17b503a

WyriHaximus requested a review from clue September 14, 2022 21:10

clue reviewed Sep 19, 2022

View reviewed changes

clue closed this Oct 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Expose DuplexStreamInterface for connections that requires Upgrades #382

[WIP] Expose DuplexStreamInterface for connections that requires Upgrades #382

bosunski commented Jul 27, 2020 •

edited

bosunski commented Jul 27, 2020 •

edited

clue left a comment

clue Aug 11, 2020

bosunski Aug 20, 2020

clue Aug 11, 2020

bosunski Aug 20, 2020

clue Aug 11, 2020

bosunski Aug 20, 2020

clue Aug 11, 2020

bosunski Aug 20, 2020

bosunski commented Aug 31, 2022

clue Sep 19, 2022

clue Sep 19, 2022

clue Sep 19, 2022

clue commented Sep 19, 2022

bosunski commented Sep 22, 2022

clue commented Oct 4, 2022

[WIP] Expose DuplexStreamInterface for connections that requires Upgrades #382

[WIP] Expose DuplexStreamInterface for connections that requires Upgrades #382

Conversation

bosunski commented Jul 27, 2020 • edited

Summary of changes

bosunski commented Jul 27, 2020 • edited

clue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bosunski commented Aug 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clue commented Sep 19, 2022

bosunski commented Sep 22, 2022

clue commented Oct 4, 2022

bosunski commented Jul 27, 2020 •

edited

bosunski commented Jul 27, 2020 •

edited