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

Fix concurrent next middleware request handlers and recurse next request handlers #293

Merged
merged 1 commit into from Dec 31, 2017

Conversation

@clue
Copy link
Member

@clue clue commented Dec 30, 2017

This PR fixes concurrently calling the next middleware request handler multiple times. Admittedly, while allowed, this is a rather uncommon situation.

Also, while I'm at it, this PR also avoids wrapping the return value from the request handler in another promise. The request handler will either return a promise or a value that can be consumed directly. When running example 99 with included benchmarking instructions, this simple change improves performance from ~2500 req/s to ~2600 req/s on my local machine. Also, running the (very synthetic) tests/benchmark-middleware-runner.php improved from ~12s to ~2s on my local machine.

Promise cancellation for pending middleware handlers was previously untested, but I did not encounter any issues here. I've added related tests to achieve 100% test coverage for this class.

Builds on top of #292

@clue clue added this to the v0.8.1 milestone Dec 30, 2017
@WyriHaximus WyriHaximus requested review from WyriHaximus and jsor Dec 30, 2017
Copy link
Member

@WyriHaximus WyriHaximus left a comment

Nice well done, that is a huge improvement!

image

jsor
jsor approved these changes Dec 30, 2017
@jsor
Copy link
Member

@jsor jsor commented Dec 31, 2017

@clue Test failing for 5.3

@clue clue force-pushed the parallel-next branch from d9590d1 to da623f3 Dec 31, 2017
@clue
Copy link
Member Author

@clue clue commented Dec 31, 2017

Updated to use $that reference :shipit:

@WyriHaximus
Copy link
Member

@WyriHaximus WyriHaximus commented Dec 31, 2017

👍

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

Successfully merging this pull request may close these issues.

None yet

3 participants