Skip to content

Simplify the code a bit using async/await #159

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
merged 9 commits into from
Apr 9, 2022
Merged

Conversation

papandreou
Copy link
Collaborator

@papandreou papandreou commented Apr 9, 2022

I've had a bit of trouble wrapping my head around exactly what's going on in index.js. Adopting async/await seems to help a bit. Added some drive-by optimizations as well so more stuff is precomputed outside the middleware itself.

This makes the middleware function async, but it doesn't ever return a rejected promise because its body is wrapped in a try/catch. This should still be compatible with both vanilla express and express-promise-router.

Drive-by fixes #153

@AndrewKeig, this is a bit of an invasive refactoring, so I would appreciate a sign-off from you 😬 🤞

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@papandreou papandreou self-assigned this Apr 9, 2022
@AndrewKeig
Copy link
Owner

Hey, I’m cool with it as long as no impact on performance, see the load tests to confirm any issues

@AndrewKeig
Copy link
Owner

Also, maybe bump the major version as this might break old versions of node, thanks for supporting this by the way, much appreciated

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@papandreou
Copy link
Collaborator Author

Tried running the load tests with node.js 16.14.2 before and after applying this change:

Before

[express-validation]$ ./load/post.sh 
Running 10s test @ http://localhost:3000/login
10 connections


┌─────────┬──────┬──────┬───────┬──────┬─────────┬─────────┬───────┐
│ Stat    │ 2.5% │ 50%  │ 97.5% │ 99%  │ Avg     │ Stdev   │ Max   │
├─────────┼──────┼──────┼───────┼──────┼─────────┼─────────┼───────┤
│ Latency │ 2 ms │ 2 ms │ 5 ms  │ 7 ms │ 2.43 ms │ 1.06 ms │ 28 ms │
└─────────┴──────┴──────┴───────┴──────┴─────────┴─────────┴───────┘
┌───────────┬────────┬────────┬─────────┬─────────┬─────────┬────────┬────────┐
│ Stat      │ 1%     │ 2.5%   │ 50%     │ 97.5%   │ Avg     │ Stdev  │ Min    │
├───────────┼────────┼────────┼─────────┼─────────┼─────────┼────────┼────────┤
│ Req/Sec   │ 1794   │ 1794   │ 3625    │ 3721    │ 3363.82 │ 561.74 │ 1794   │
├───────────┼────────┼────────┼─────────┼─────────┼─────────┼────────┼────────┤
│ Bytes/Sec │ 858 kB │ 858 kB │ 1.73 MB │ 1.78 MB │ 1.61 MB │ 269 kB │ 858 kB │
└───────────┴────────┴────────┴─────────┴─────────┴─────────┴────────┴────────┘

Req/Bytes counts sampled once per second.
# of samples: 11

0 2xx responses, 37000 non 2xx responses
37k requests in 11.02s, 17.7 MB read

After

[express-validation]$ ./load/post.sh 
Running 10s test @ http://localhost:3000/login
10 connections


┌─────────┬──────┬──────┬───────┬──────┬────────┬─────────┬───────┐
│ Stat    │ 2.5% │ 50%  │ 97.5% │ 99%  │ Avg    │ Stdev   │ Max   │
├─────────┼──────┼──────┼───────┼──────┼────────┼─────────┼───────┤
│ Latency │ 2 ms │ 2 ms │ 5 ms  │ 7 ms │ 2.4 ms │ 1.05 ms │ 26 ms │
└─────────┴──────┴──────┴───────┴──────┴────────┴─────────┴───────┘
┌───────────┬────────┬────────┬─────────┬─────────┬─────────┬────────┬────────┐
│ Stat      │ 1%     │ 2.5%   │ 50%     │ 97.5%   │ Avg     │ Stdev  │ Min    │
├───────────┼────────┼────────┼─────────┼─────────┼─────────┼────────┼────────┤
│ Req/Sec   │ 1804   │ 1804   │ 3667    │ 3833    │ 3420.9  │ 623.18 │ 1804   │
├───────────┼────────┼────────┼─────────┼─────────┼─────────┼────────┼────────┤
│ Bytes/Sec │ 863 kB │ 863 kB │ 1.75 MB │ 1.83 MB │ 1.63 MB │ 298 kB │ 862 kB │
└───────────┴────────┴────────┴─────────┴─────────┴─────────┴────────┴────────┘

Req/Bytes counts sampled once per second.
# of samples: 10

0 2xx responses, 34206 non 2xx responses
34k requests in 10.02s, 16.4 MB read

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@papandreou papandreou merged commit 95d9352 into master Apr 9, 2022
@papandreou papandreou deleted the tech/asyncAwait branch April 9, 2022 12:39
@papandreou
Copy link
Collaborator Author

Released in 4.0.0 🚀

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

Successfully merging this pull request may close these issues.

joiOptions context not found
2 participants