Skip to content
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

EmptyBodyStream as request body not working #497

Closed
martin-heralecky opened this issue Jun 2, 2023 · 3 comments · Fixed by #516
Closed

EmptyBodyStream as request body not working #497

martin-heralecky opened this issue Jun 2, 2023 · 3 comments · Fixed by #516
Labels
Milestone

Comments

@martin-heralecky
Copy link

martin-heralecky commented Jun 2, 2023

There seems to be a problem when attempting to pass EmptyBodyStream as body to HTTP requests:

(new Browser())
    ->requestStreaming("GET", "https://stackoverflow.com", [], new \React\Http\Io\EmptyBodyStream())
    ->then(var_dump(...)); // never get's triggered

Whereas, empty resource works just fine:

(new Browser())
    ->requestStreaming("GET", "https://stackoverflow.com", [], new ReadableResourceStream(tmpfile()))
    ->then(var_dump(...));

I have tried other servers (URLs) as well, and for some of them I get a response immediatelly, for some there is a 10s delay. Maybe the socket isn't being closed when using EmptyBodyStream?


The full example (reason I care) is that I though this would work as a simple HTTP proxy:

$browser = new Browser();
$browser = $browser->withRejectErrorResponse(false);

$server = new HttpServer(
    new StreamingRequestMiddleware(),
    fn(ServerRequestInterface $req) => $browser
        ->requestStreaming($req->getMethod(), TARGET_URL, [], $req->getBody())
        ->then(fn(ResponseInterface $res) => new Response($res->getStatusCode(), [], $res->getBody())),
);

But it only works for requests with non-empty bodies. Otherwise $req->getBody() is EmptyBodyStream and the requests get stuck.

@SimonFrings
Copy link
Member

Hey @martin-heralecky, thanks for bringing this up 👍

I talked with @clue about this, and it indeed seems to be a bug that has found its way into the project over the last few releases. We've already started working on a fix for this, so I will keep you updated as we make progress.

As you also may know, we're currently working on ReactPHP v3 and releasing the roadmap tickets for each component (see reactphp/event-loop#271 and others). The HTTP roadmap ticket will also be released soon, and to give you a sneak peek, we plan to release at least v1.10.0 before branching out with a 3.x branch. So, I think it makes sense to resolve this bug as part of the upcoming v1.1.0.0 and thus also for the upcoming v3.

@clue
Copy link
Member

clue commented Feb 22, 2024

@martin-heralecky Again thank you for reporting, that was a nasty one! I've just filed #516 which fixes the problem and introduces additional tests to avoid any future regressions.

I've double-checked your gist, and I'm happy to confirm that it now works flawlessly with the applied changes. The fix may actually look surprisingly simple, but analyzing the problem and coming up with this fix took a couple of hours. If you'd like to support this development, consider sponsoring ReactPHP! ❤️ Thank you!

@martin-heralecky
Copy link
Author

Thanks a lot! I'll definitely consider sponsoring the project. You guys are doing an excelent job.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants