Skip to content

Don't run requests through configuration middleware #422

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

Merged

Conversation

WyriHaximus
Copy link
Member

While working on another PR that introduces another configuration middleware it dawned on me that we preferably don't requests through middleware that don't do anything.

This change will filter it out those middleware before they are passed into the MiddlewareRunner.

While running benchmarks I gained around a 10% requests per second gain when running it against example 63.

@WyriHaximus WyriHaximus added this to the v1.6.0 milestone Aug 17, 2021
@WyriHaximus WyriHaximus force-pushed the dont-run-configuration-middleware branch from e9e2342 to fdd77ad Compare August 17, 2021 19:11
@WyriHaximus WyriHaximus requested review from clue and jsor August 17, 2021 19:13
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.

Changes make perfect sense, added some minor remarks below 👍

While working on another PR that introduces another configuration middleware it dawned on me that we preferably don't requests through middleware that don't do anything.

This change will filter it out those middleware before they are passed into the MiddlewareRunner.

While running benchmarks I gained around a 10% requests per second gain when running it against example 63.
@WyriHaximus WyriHaximus force-pushed the dont-run-configuration-middleware branch from fdd77ad to 871edea Compare August 20, 2021 14:31
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.

@WyriHaximus Thanks for the update, changes LGTM! :shipit:

Hm, just noticed the build currently fails, can you look into this?

@clue clue self-requested a review August 20, 2021 16:44
@WyriHaximus
Copy link
Member Author

Hm, just noticed the build currently fails, can you look into this?

Already looking into that, not sure why they suddenly started failing, because they now also fail locally, even when I revert the changes 0_o

@WyriHaximus
Copy link
Member Author

@clue Both locally and on GitHub that tests now runs fine, without any changes 🤯 . (Restarted it 5 times or so, green every time now.)

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.

@WyriHaximus Sounds good to me, looks like we should look into the flaky test in a separate PR, looks like it's unrelated anyway 👍

@clue clue merged commit 29940dc into reactphp:master Aug 20, 2021
@WyriHaximus WyriHaximus deleted the dont-run-configuration-middleware branch August 20, 2021 19:42
@WyriHaximus
Copy link
Member Author

@clue Agreed, although I'm not sure why that test took more then 10 seconds

WyriHaximus added a commit to WyriHaximus-labs/http that referenced this pull request Aug 22, 2021
This new middleware introduces a timeout of closing inactive
connections between connections after a configured amount of seconds.

This builds on top of reactphp#405 and partially on reactphp#422
WyriHaximus added a commit to WyriHaximus-labs/http that referenced this pull request Aug 22, 2021
This new middleware introduces a timeout of closing inactive
connections between connections after a configured amount of seconds.

This builds on top of reactphp#405 and partially on reactphp#422
WyriHaximus added a commit to WyriHaximus-labs/http that referenced this pull request Aug 30, 2021
This new middleware introduces a timeout of closing inactive
connections between connections after a configured amount of seconds.

This builds on top of reactphp#405 and partially on reactphp#422
WyriHaximus added a commit to WyriHaximus-labs/http that referenced this pull request Nov 9, 2021
This new middleware introduces a timeout of closing inactive
connections between connections after a configured amount of seconds.

This builds on top of reactphp#405 and partially on reactphp#422
WyriHaximus added a commit to WyriHaximus-labs/http that referenced this pull request Nov 9, 2021
This new middleware introduces a timeout of closing inactive
connections between connections after a configured amount of seconds.

This builds on top of reactphp#405 and partially on reactphp#422
WyriHaximus added a commit to WyriHaximus-labs/http that referenced this pull request Mar 22, 2022
This new middleware introduces a timeout of closing inactive
connections between connections after a configured amount of seconds.

This builds on top of reactphp#405 and partially on reactphp#422
WyriHaximus added a commit to WyriHaximus-labs/http that referenced this pull request Aug 16, 2022
This new middleware introduces a timeout of closing inactive
connections between connections after a configured amount of seconds.

This builds on top of reactphp#405 and partially on reactphp#422
WyriHaximus added a commit to WyriHaximus-labs/http that referenced this pull request Aug 27, 2022
This new middleware introduces a timeout of closing inactive
connections between connections after a configured amount of seconds.

This builds on top of reactphp#405 and partially on reactphp#422
WyriHaximus added a commit to WyriHaximus-labs/http that referenced this pull request Sep 14, 2022
This new middleware introduces a timeout of closing inactive
connections between connections after a configured amount of seconds.

This builds on top of reactphp#405 and partially on reactphp#422
WyriHaximus added a commit to WyriHaximus-labs/http that referenced this pull request Sep 15, 2022
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 added a commit to WyriHaximus-labs/http that referenced this pull request Mar 25, 2024
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 added a commit to WyriHaximus-labs/http that referenced this pull request Mar 25, 2024
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 added a commit to WyriHaximus-labs/http that referenced this pull request Mar 25, 2024
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 added a commit to WyriHaximus-labs/http that referenced this pull request Mar 26, 2024
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 added a commit to WyriHaximus-labs/http that referenced this pull request Mar 26, 2024
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 added a commit to WyriHaximus-labs/http that referenced this pull request Mar 26, 2024
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 added a commit to WyriHaximus-labs/http that referenced this pull request Mar 26, 2024
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 added a commit to WyriHaximus-labs/http that referenced this pull request Mar 26, 2024
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 added a commit to WyriHaximus-labs/http that referenced this pull request Mar 30, 2024
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 added a commit to WyriHaximus-labs/http that referenced this pull request Mar 30, 2024
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 added a commit to WyriHaximus-labs/http that referenced this pull request Mar 30, 2024
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 added a commit to WyriHaximus-labs/http that referenced this pull request Mar 30, 2024
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 added a commit to WyriHaximus-labs/http that referenced this pull request Apr 6, 2024
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 added a commit to WyriHaximus-labs/http that referenced this pull request Apr 10, 2024
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 added a commit to WyriHaximus-labs/http that referenced this pull request Apr 10, 2024
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
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.

2 participants