amphp / http-server

An advanced async HTTP server library for PHP, perfect for real-time apps and APIs with high concurrency demands.
https://amphp.org/http-server
MIT License
1.29k stars 101 forks source link

HTTP/2 end of stream event missing #343

Closed Nek- closed 1 year ago

Nek- commented 1 year ago

It is not possible to execute some code when an http/2 stream ends. (it http 1 it's connection closed, but in http/2 the connection do not close at end of request)

Notice: @trowski is already aware of this issue but we spoke about it several days ago. So to keep track of it I open an issue.

trowski commented 1 year ago

You must have missed my replies in chat. I tried to replicate the issue with and without compression, but was unable to. Using the event-stream.php example modified to enable encrypt for HTTP/2, Response::onDispose() is invoked immediately after the RST_STREAM frame is received from the browser.

I wonder if you're somehow holding onto the reference to the Response object in your app?

Rather than relying on the destructor of Response, we should be explicitly invoking a method when the response is sent to help avoid an issue in case an app creates a circular reference?

Nek- commented 1 year ago

Hum I see, the error may be related to how PHP is removing the item from memory. But no, I don't have any reference to this object. I'll take a look deeper to see if I can trigger the garbage collector manually in a good place.

(for the record, the code is here: https://github.com/swagindustries/drumkit/blob/f3ee6ed4b1c4bab06e9d7e9f9d572e0f23d10c2e/src/Controller/SubscribeController.php#L63 )

trowski commented 1 year ago

I'm not 100% sure what may be going on in a more complex app. Perhaps the server is holding onto the reference? I looked at setting up your app but was missing some components so I tried a simpler setup. Would you be able to simplify the setup so I don't have to have another service running to trigger events?

trowski commented 1 year ago

Could you please push a branch of your project with the version where you're setting an onDispose callback on the Request?

Nek- commented 1 year ago

Would you be able to simplify the setup so I don't have to have another service running to trigger events?

Once running (you need a custom certificate, the makefile helps you) you can use the app itself to trigger event by going to https://localhost . I'll add a new branch with the onDispose implementation this evening. 👍

Nek- commented 1 year ago

The following code works great on the latest beta:

$response->onDispose(function () { /* do something on end of stream */ });
trowski commented 1 year ago

This was fixed in https://github.com/amphp/http-server/commit/d579d5bd521ab74989e61259b3ded48b48baf094, but I forgot to close the issue at the time. Thanks for confirming @Nek-!