Skip to content

Establish reply.sent = true as a proper way to skip async/await handling#1336

Merged
mcollina merged 1 commit intomasterfrom
sent-refactor
Dec 20, 2018
Merged

Establish reply.sent = true as a proper way to skip async/await handling#1336
mcollina merged 1 commit intomasterfrom
sent-refactor

Conversation

@mcollina
Copy link
Copy Markdown
Member

@mcollina mcollina commented Dec 19, 2018

Fixes #1330

To avoid conflicts, this is based on #1328, and it will need to be rebased once that lands.

Checklist

  • run npm run test and npm run benchmark
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message and code follows Code of conduct

@mcollina mcollina requested a review from delvedor December 19, 2018 08:47
This was referenced Dec 19, 2018
Closed
Merged
delvedor
delvedor reviewed Dec 19, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@delvedor delvedor left a comment

Choose a reason for hiding this comment

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

Great work! I left a couple of comments.

test/validation-error-handling.test.js

const fastify = Fastify()

fastify.get('/', { schema: { response } }, function (req, reply) {
Copy link
Copy Markdown
Member

@delvedor delvedor Dec 19, 2018

Choose a reason for hiding this comment

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

Can you add a test with promises as well?

Copy link
Copy Markdown
Member Author

@mcollina mcollina Dec 19, 2018

Choose a reason for hiding this comment

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

This should be included in #1328

@delvedor delvedor added bugfix Issue or PR that should land as semver patch backport 1.x Issue or pr that should be backported to Fastify v1 labels Dec 19, 2018
@mcollina
Copy link
Copy Markdown
Member Author

mcollina commented Dec 19, 2018

I've rebased on top of master. PTAL.

delvedor
delvedor approved these changes Dec 19, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@delvedor delvedor left a comment

Choose a reason for hiding this comment

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

LGTM

cemremengu
cemremengu approved these changes Dec 19, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@cemremengu cemremengu left a comment

Choose a reason for hiding this comment

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

LGTM

@allevo
Copy link
Copy Markdown
Member

allevo commented Dec 19, 2018

Can you run some benchmarks?

@mcollina
Copy link
Copy Markdown
Member Author

mcollina commented Dec 19, 2018

sent-refactor:


$ autocannon -c 100 -d 5 -p 10 localhost:3000
Running 5s test @ http://localhost:3000
100 connections with 10 pipelining factor

┌─────────┬──────┬──────┬───────┬──────┬─────────┬─────────┬──────────┐
│ Stat    │ 2.5% │ 50%  │ 97.5% │ 99%  │ Avg     │ Stdev   │ Max      │
├─────────┼──────┼──────┼───────┼──────┼─────────┼─────────┼──────────┤
│ Latency │ 0 ms │ 0 ms │ 9 ms  │ 9 ms │ 1.19 ms │ 3.03 ms │ 39.73 ms │
└─────────┴──────┴──────┴───────┴──────┴─────────┴─────────┴──────────┘
┌───────────┬─────────┬─────────┬───────┬─────────┬─────────┬────────┬─────────┐
│ Stat      │ 1%      │ 2.5%    │ 50%   │ 97.5%   │ Avg     │ Stdev  │ Min     │
├───────────┼─────────┼─────────┼───────┼─────────┼─────────┼────────┼─────────┤
│ Req/Sec   │ 75775   │ 75775   │ 79231 │ 80319   │ 78854.4 │ 1637   │ 75774   │
├───────────┼─────────┼─────────┼───────┼─────────┼─────────┼────────┼─────────┤
│ Bytes/Sec │ 12.4 MB │ 12.4 MB │ 13 MB │ 13.2 MB │ 12.9 MB │ 267 kB │ 12.4 MB │
└───────────┴─────────┴─────────┴───────┴─────────┴─────────┴────────┴─────────┘

Req/Bytes counts sampled once per second.

394k requests in 5.04s, 64.7 MB read

master:

$ autocannon -c 100 -d 5 -p 10 localhost:3000
Running 5s test @ http://localhost:3000
100 connections with 10 pipelining factor

┌─────────┬──────┬──────┬───────┬──────┬─────────┬─────────┬──────────┐
│ Stat    │ 2.5% │ 50%  │ 97.5% │ 99%  │ Avg     │ Stdev   │ Max      │
├─────────┼──────┼──────┼───────┼──────┼─────────┼─────────┼──────────┤
│ Latency │ 0 ms │ 0 ms │ 9 ms  │ 9 ms │ 1.19 ms │ 3.03 ms │ 48.69 ms │
└─────────┴──────┴──────┴───────┴──────┴─────────┴─────────┴──────────┘
┌───────────┬─────────┬─────────┬─────────┬─────────┬─────────┬─────────┬─────────┐
│ Stat      │ 1%      │ 2.5%    │ 50%     │ 97.5%   │ Avg     │ Stdev   │ Min     │
├───────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│ Req/Sec   │ 76543   │ 76543   │ 78783   │ 80703   │ 78726.4 │ 1324.42 │ 76510   │
├───────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│ Bytes/Sec │ 12.6 MB │ 12.6 MB │ 12.9 MB │ 13.2 MB │ 12.9 MB │ 217 kB  │ 12.5 MB │
└───────────┴─────────┴─────────┴─────────┴─────────┴─────────┴─────────┴─────────┘

Req/Bytes counts sampled once per second.

394k requests in 5.04s, 64.6 MB read

allevo
allevo approved these changes Dec 19, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@allevo allevo left a comment

Choose a reason for hiding this comment

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

LGTM

@mcollina mcollina merged commit 09becaf into master Dec 20, 2018
@mcollina mcollina deleted the sent-refactor branch December 20, 2018 09:03
@fralonra fralonra mentioned this pull request Dec 20, 2018
Closed
21 tasks
mcollina added a commit that referenced this pull request Dec 20, 2018
@mcollina
Establish reply.sent = true as a proper way to skip async/await handl…
f0db428 
…ing (#1336)

Fixes: #1330.
@Ethan-Arrowood Ethan-Arrowood mentioned this pull request Jan 4, 2019
Closed
@fralonra fralonra mentioned this pull request Jan 19, 2019
Merged
@github-actions
Copy link
Copy Markdown

github-actions bot commented Feb 12, 2022

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@delvedor delvedor delvedor approved these changes

+2 more reviewers

@allevo allevo allevo approved these changes

@cemremengu cemremengu cemremengu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

backport 1.x Issue or pr that should be backported to Fastify v1 bugfix Issue or PR that should land as semver patch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mcollina @allevo @cemremengu @delvedor