Close inactive connections and requests #425

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

Jump to bottom

Open

WyriHaximus wants to merge 2 commits into reactphp:1.x

from WyriHaximus-labs:close-inactive-requests

Open

Close inactive connections and requests #425

WyriHaximus wants to merge 2 commits into reactphp:1.x from WyriHaximus-labs:close-inactive-requests

Conversation

@WyriHaximus

Copy link

Member

@WyriHaximus WyriHaximus commented Sep 1, 2021 •

edited

Loading

This builds on top of #405 and further builds out #423 by also
close connections with inactive requests.

Closes: #423

@WyriHaximus WyriHaximus added new feature HTTP/1.1 labels

Sep 1, 2021

@WyriHaximus WyriHaximus added this to the v1.6.0 milestone

Sep 1, 2021

@WyriHaximus WyriHaximus changed the title ~~(削除) Close inactive connections (削除ここまで)~~ (追記) Close inactive requests (追記ここまで)

Sep 1, 2021

@WyriHaximus WyriHaximus mentioned this pull request

Sep 1, 2021

Close inactive connections #423

Closed

@clue clue mentioned this pull request

Oct 7, 2021

Support persistent connections (Connection: keep-alive) #405

Merged

@mmoreram

Copy link

mmoreram commented Nov 9, 2021

@WyriHaximus is there any chance you can merge this PR? Seems to be tested, and increases so much the memory performance :)

@WyriHaximus WyriHaximus force-pushed the close-inactive-requests branch 2 times, most recently from 7b3f5b6 to 0ddb503 Compare

November 10, 2021 07:13

@WyriHaximus

Copy link

Member Author

WyriHaximus commented Nov 10, 2021

@WyriHaximus Thanks for the update!

Can you provide a quick update here, what's your plan for this feature with regards to #425? We've briefly discussed (off channel) that this should eventually send a 408 Request Timeout response for the initial connection but not for keep-alive connections that send no following request. I agree breaking this up into smaller PRs can be useful, but it looks like this might also work in a single PR, so I'll leave this up to you. Let me know if there's anything I can help with and/or if you want me to take over from here +1

@clue Right, been thinking about this, and doesn't it make sense to do this with all requests in flight, instead of only on the first?

@WyriHaximus WyriHaximus force-pushed the close-inactive-requests branch from 0ddb503 to 4af0447 Compare

November 10, 2021 07:23

@WyriHaximus

Copy link

Member Author

WyriHaximus commented Nov 10, 2021

@WyriHaximus is there any chance you can merge this PR? Seems to be tested, and increases so much the memory performance :)

@mmoreram There are a few details we need to polish before this PR is done and it can be merged. Like the 408 responses for when an inflight request times out. And ensuring we don't cut off websocket connections with this.

@WyriHaximus WyriHaximus force-pushed the close-inactive-requests branch from 4af0447 to 8d4d62c Compare

November 10, 2021 07:28

@WyriHaximus

Copy link

Member Author

WyriHaximus commented Nov 10, 2021

@WyriHaximus Thanks for the update!
Can you provide a quick update here, what's your plan for this feature with regards to #425? We've briefly discussed (off channel) that this should eventually send a 408 Request Timeout response for the initial connection but not for keep-alive connections that send no following request. I agree breaking this up into smaller PRs can be useful, but it looks like this might also work in a single PR, so I'll leave this up to you. Let me know if there's anything I can help with and/or if you want me to take over from here +1

@clue Right, been thinking about this, and doesn't it make sense to do this with all requests in flight, instead of only on the first?

Just made this change, the code is cleaner as well. I Will check the relevant RFC's later today, but until that my position is that any request in a keep alive connection can timeout for any number of reasons.

@clue clue removed this from the v1.6.0 milestone

Feb 3, 2022

clue

clue reviewed

Feb 5, 2022

View reviewed changes

src/Io/StreamingServer.php Outdated

$this->callback = $requestHandler;

$this->parser = new RequestHeaderParser();

$this->parser = new RequestHeaderParser($this->loop, $this->idleConnectionTimeout);

Copy link

Member

@clue clue Feb 5, 2022

Choose a reason for hiding this comment

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

Makes me wonder if we should expose the RequestHeaderParser instead, given we only use the timeout value to pass it forward. (Refs #359)

Copy link

Member Author

@WyriHaximus WyriHaximus Mar 17, 2022

Choose a reason for hiding this comment

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

As in also make it possible to inject a different header parser? We could do that so we can have a C based extension for parser to speed it up?

Copy link

Member

@clue clue Mar 17, 2022

Choose a reason for hiding this comment

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

KISS: The StreamingServer is marked @internal, as is the RequestHeaderParser. This means we can easily inject the RequestHeaderParser to the StreamingServer inside the Server class. Constructor may have to be changed, but doesn't affect BC, because the public Server interface is not affected (at least not as part of this change).

Copy link

Member Author

@WyriHaximus WyriHaximus Mar 21, 2022

Choose a reason for hiding this comment

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

Yes, I'm fully aware, I'm also thinking ahead. But maybe, if we want to go there, we should ship that C based extension to speed it up

Copy link

Member

@clue clue Mar 21, 2022

Choose a reason for hiding this comment

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

I'm not sure this is currently our hot path, but if we have evidence an optional extension will speed things up, I'm all for it 👍 I think we're on the same page here and should probably leave this up for a follow-up PR?

In either case I think your changes are a welcome improvement as is and injecting the RequestHeaderParser into the StreamingServer can help simplify the dependency injection both now and longer-term 👍

Copy link

Member Author

@WyriHaximus WyriHaximus Mar 21, 2022 •

edited

Loading

Choose a reason for hiding this comment

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

I'm not sure this is currently our hot path, but if we have evidence an optional extension will speed things up, I'm all for it 👍 I think we're on the same page here and should probably leave this up for a follow-up PR?

Of course, that would be a follow-up PR. And we have tons of other things to do.

In either case I think your changes are a welcome improvement as is and injecting the RequestHeaderParser into the StreamingServer can help simplify the dependency injection both now and longer-term 👍

Sounds good! 🚢 🇮🇹 !

(Will fix that thing below this now.)

Copy link

Member

@clue clue Mar 29, 2022

Choose a reason for hiding this comment

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

It's my understanding the HttpServer should be simplified to call something like new StreamingServer($middleware, new RequestHeaderParser($loop, $idleConnectionTimeout)).

Copy link

Member Author

@WyriHaximus WyriHaximus Mar 29, 2022

Choose a reason for hiding this comment

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

It took some work, but I think it's worth the refactoring ❤️

Copy link

Member Author

@WyriHaximus WyriHaximus Apr 10, 2024

Choose a reason for hiding this comment

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

Took some working on this, but ended up with new StreamingServer($middleware, $idleConnectionTimeout) because the connection/requests timing out is handled in the StreamingServer.

src/Io/RequestHeaderParser.php Outdated Show resolved Hide resolved

@clue

Copy link

Member

clue commented Feb 5, 2022

@WyriHaximus Thanks for the update!
Can you provide a quick update here, what's your plan for this feature with regards to #425? We've briefly discussed (off channel) that this should eventually send a 408 Request Timeout response for the initial connection but not for keep-alive connections that send no following request. I agree breaking this up into smaller PRs can be useful, but it looks like this might also work in a single PR, so I'll leave this up to you. Let me know if there's anything I can help with and/or if you want me to take over from here +1

@clue Right, been thinking about this, and doesn't it make sense to do this with all requests in flight, instead of only on the first?

Just made this change, the code is cleaner as well. I Will check the relevant RFC's later today, but until that my position is that any request in a keep alive connection can timeout for any number of reasons.

Did take a look at the relevant RFCs (https://datatracker.ietf.org/doc/html/rfc7230#section-6 and https://datatracker.ietf.org/doc/html/rfc7231#section-6.5.7 come to mind) and it looks like this is indeed under-specified behavior. MDN mentions "Note: some servers merely shut down the connection without sending this message." (https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/408) and this actually seems to match my observations.

If you open a connection github.com:80 or google.com:80 and don't send a complete request, the connection will be closed with no message after a timeout. Same if you send a request using keep-alive and then wait for the timeout. Same if you send a request using keep-alive and then send an incomplete request header.
If you open a connection to Apache and don't send any data, the connection will be closed with no message after a timeout. If you open a connection and send an incomplete request header, the connection will be closed with a message after a timeout. In either case, a 408 will be logged. If you send a request using keep-alive and then wait for the timeout, the connection will be closed with no message after a timeout and no log message. If you send a request using keep-alive and then send an incomplete request header, the connection will be closed with an message after a timeout and a log message.
If you open a connection to nginx, it will close the connection with no message after after a timeout in either case. Even though I'm using default settings (http://nginx.org/en/docs/http/ngx_http_core_module.html#client_header_timeout), I've never seen an 408 response in my tests.
Note there's also https://www.haproxy.com/blog/haproxy-and-http-errors-408-in-chrome/ which suggests sending a 408 used to be(?) a problem in chrome to its inherent race condition. See also https://stackoverflow.com/questions/42631273/how-do-browsers-handle-http-keepalive-race-condition for some background.

Neither behavior appears to be against specs, so I wonder what behavior we should use here.

@WyriHaximus WyriHaximus force-pushed the close-inactive-requests branch 4 times, most recently from 2684f7d to ec39c37 Compare

March 22, 2022 14:32

@WyriHaximus

Copy link

Member Author

WyriHaximus commented Mar 23, 2022

Neither behavior appears to be against specs, so I wonder what behavior we should use here.

@clue For now, merge this as is and I'll do a follow up PR to make the behavior configurable.

@WyriHaximus WyriHaximus requested review from clue and jsor

March 23, 2022 13:35

clue

clue requested changes

Mar 29, 2022

View reviewed changes

src/Io/RequestHeaderParser.php Outdated Show resolved Hide resolved

src/Middleware/InactiveConnectionTimeoutMiddleware.php Show resolved Hide resolved

src/Io/StreamingServer.php Outdated Show resolved Hide resolved

src/Io/StreamingServer.php Show resolved Hide resolved

src/Io/StreamingServer.php Outdated Show resolved Hide resolved

src/Io/StreamingServer.php Outdated

$this->callback = $requestHandler;

$this->parser = new RequestHeaderParser();

$this->parser = new RequestHeaderParser($this->loop, $this->idleConnectionTimeout);

Copy link

Member

@clue clue Mar 29, 2022

Choose a reason for hiding this comment

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

It's my understanding the HttpServer should be simplified to call something like new StreamingServer($middleware, new RequestHeaderParser($loop, $idleConnectionTimeout)).

@WyriHaximus WyriHaximus force-pushed the close-inactive-requests branch 3 times, most recently from 3db8005 to 4c481fa Compare

March 30, 2022 07:05

@WyriHaximus WyriHaximus force-pushed the close-inactive-requests branch 2 times, most recently from 1de8cd4 to 4f47478 Compare

August 17, 2022 06:56

@WyriHaximus WyriHaximus force-pushed the close-inactive-requests branch from efc3762 to 7407d82 Compare

September 14, 2022 17:27

@WyriHaximus WyriHaximus requested a review from clue

September 14, 2022 18:03

@WyriHaximus WyriHaximus force-pushed the close-inactive-requests branch from 7407d82 to 3398ca8 Compare

September 14, 2022 18:05

@WyriHaximus WyriHaximus changed the title ~~(削除) Close inactive requests (削除ここまで)~~ (追記) Close inactive connections and requests (追記ここまで)

Sep 15, 2022

@WyriHaximus WyriHaximus force-pushed the close-inactive-requests branch from 3398ca8 to 25f7136 Compare

September 15, 2022 06:49

@clue clue modified the milestones: v1.8.0, v1.9.0

Sep 29, 2022

@WyriHaximus WyriHaximus modified the milestones: v1.9.0, v1.10.0

Apr 17, 2023

@WyriHaximus WyriHaximus requested a review from SimonFrings

February 17, 2024 19:21

@WyriHaximus WyriHaximus force-pushed the close-inactive-requests branch 7 times, most recently from 655ca5e to 56057dc Compare

March 26, 2024 22:43

@SimonFrings SimonFrings modified the milestones: v1.10.0, v1.11.0

Mar 27, 2024

@WyriHaximus WyriHaximus force-pushed the close-inactive-requests branch from 56057dc to d0a0442 Compare

March 30, 2024 21:19

@WyriHaximus


 Typo

cb72360

@WyriHaximus WyriHaximus force-pushed the close-inactive-requests branch 4 times, most recently from e447b34 to 6b78aa5 Compare

April 6, 2024 19:49

@WyriHaximus WyriHaximus force-pushed the close-inactive-requests branch from 6b78aa5 to 6491fab Compare

April 10, 2024 05:11

@WyriHaximus


 Close inactive connections and requests

a270175

This new middleware introduces a timeout of closing inactive
connections between requests after a configured amount of seconds.
This builds on top of reactphp#405 and partially on reactphp#422

@WyriHaximus WyriHaximus force-pushed the close-inactive-requests branch from 6491fab to a270175 Compare

April 10, 2024 05:15

@WyriHaximus WyriHaximus removed the request for review from jsor

April 10, 2024 05:22

@clue clue removed this from the v1.11.0 milestone

Oct 14, 2024

Labels

HTTP/1.1 new feature

4 participants

@WyriHaximus @mmoreram @clue @SimonFrings

Uh oh!

Close inactive connections and requests #425

Are you sure you want to change the base?

Close inactive connections and requests #425

Uh oh!

Conversation

@WyriHaximus WyriHaximus commented Sep 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmoreram commented Nov 9, 2021

Uh oh!

WyriHaximus commented Nov 10, 2021

Uh oh!

WyriHaximus commented Nov 10, 2021

Uh oh!

WyriHaximus commented Nov 10, 2021

Uh oh!

@clue clue Feb 5, 2022

Choose a reason for hiding this comment

Uh oh!

@WyriHaximus WyriHaximus Mar 17, 2022

Choose a reason for hiding this comment

Uh oh!

@clue clue Mar 17, 2022

Choose a reason for hiding this comment

Uh oh!

@WyriHaximus WyriHaximus Mar 21, 2022

Choose a reason for hiding this comment

Uh oh!

@clue clue Mar 21, 2022

Choose a reason for hiding this comment

Uh oh!

@WyriHaximus WyriHaximus Mar 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@clue clue Mar 29, 2022

Choose a reason for hiding this comment

Uh oh!

@WyriHaximus WyriHaximus Mar 29, 2022

Choose a reason for hiding this comment

Uh oh!

@WyriHaximus WyriHaximus Apr 10, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

clue commented Feb 5, 2022

Uh oh!

WyriHaximus commented Mar 23, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

@clue clue Mar 29, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

@WyriHaximus WyriHaximus commented Sep 1, 2021 •

edited

Loading

@WyriHaximus WyriHaximus Mar 21, 2022 •

edited

Loading