http icon indicating copy to clipboard operation
http copied to clipboard

[WIP] Expose DuplexStreamInterface for connections that requires Upgrades

Open bosunski opened this issue 5 years ago • 1 comments

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 avatar Jul 27 '20 20:07 bosunski

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

bosunski avatar Jul 27 '20 21:07 bosunski

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

bosunski avatar Aug 31 '22 14:08 bosunski

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?

clue avatar Sep 19 '22 16:09 clue

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.

bosunski avatar Sep 22 '22 08:09 bosunski

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

clue avatar Oct 04 '22 15:10 clue