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

Handle keepalive #76

Closed
wants to merge 1 commit into from
Closed

Handle keepalive #76

wants to merge 1 commit into from

Conversation

jason69
Copy link

@jason69 jason69 commented Oct 26, 2016

Hi,
I see a previous keepalive pull request opened for some time now.
Here is another one I made.
I find it clearer than the previous one because the connection is maintained/closed according to request header and a max number of reuse of the connection.

I've done some tests with apache benchark on a virtual machine and I'm getting an average of 1.6x requests/s with keepalive.

By the way testing with PHP 7.0.8 shows that the connexion shouldn't be reused too many times. I set it as a constant in the code to 25. Increasing this number shows a drop in speed, I havent managed to understand why...

Closes #39

Copy link
Member

@WyriHaximus WyriHaximus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good at first skim, will run test later this week 👍

@@ -6,54 +6,88 @@
use React\Socket\ServerInterface as SocketServerInterface;
use React\Socket\ConnectionInterface;


class ConnProxy {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you extract this into it's own file

@WyriHaximus WyriHaximus requested review from jsor and clue February 7, 2017 22:18
Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for filing this PR, I'd really like to see this feature in 👍

Unfortunately I do not think this is reasonable in its current state, given that we have not implemented chunked transfer encoding (#96) yet.

Connection reuse requires being able to detect exact message boundaries, which in turn requires handling explicit message lengths, chunked transfer encoding and implicit message lengths. (This is covered in detail in https://tools.ietf.org/html/rfc7230#section-3.3)

As much as I'd like to get this feature in, I'm not sure reviewing this in depth currently makes much sense 👍

@clue
Copy link
Member

clue commented Dec 12, 2017

As much as I'd love to get this feature in, I'm having to close this for now as it hasn't received any input in a while and it's unlikely this will get traction any time soon.

The feature request is still open in #39 and we'll look into this in the not too far future 👍 If you feel this was closed prematurely or want to pick this up again, please let us know and we can reopen this.

Thank you for your effort!

@clue clue closed this Dec 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support persistent connections (aka HTTP/1.1 Keep-Alive)
3 participants