feat: Web Stream API by climba03003 · Pull Request #5286 · fastify/fastify

climba03003 · 2024-01-25T07:35:31Z

Closes #5280 Closes #5279

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 the Developer's Certification of Origin
and the Code of conduct

lib/reply.js

mcollina

Could you check if this impacts our benchmarks? I fear of those long optional checks in .send().

lib/reply.js

climba03003 · 2024-01-25T09:21:49Z

Pull Request

> fastify@4.25.2 benchmark
> concurrently -k -s first "node ./examples/benchmark/simple.js" "autocannon -c 100 -d 30 -p 10 localhost:3000/"

[1] Running 30s test @ http://localhost:3000/
[1] 100 connections with 10 pipelining factor
[1]
[1] 
[1] ┌─────────┬──────┬───────┬───────┬────────┬──────────┬──────────┬────────┐
[1] │ Stat    │ 2.5% │ 50%   │ 97.5% │ 99%    │ Avg      │ Stdev    │ Max    │
[1] ├─────────┼──────┼───────┼───────┼────────┼──────────┼──────────┼────────┤
[1] │ Latency │ 7 ms │ 18 ms │ 95 ms │ 140 ms │ 21.95 ms │ 20.46 ms │ 201 ms │
[1] └─────────┴──────┴───────┴───────┴────────┴──────────┴──────────┴────────┘
[1] ┌───────────┬─────────┬─────────┬────────┬────────┬──────────┬──────────┬─────────┐
[1] │ Stat      │ 1%      │ 2.5%    │ 50%    │ 97.5%  │ Avg      │ Stdev    │ Min     │
[1] ├───────────┼─────────┼─────────┼────────┼────────┼──────────┼──────────┼─────────┤
[1] │ Req/Sec   │ 38,143  │ 38,143  │ 44,735 │ 47,327 │ 44,534.4 │ 2,041.82 │ 38,112  │
[1] ├───────────┼─────────┼─────────┼────────┼────────┼──────────┼──────────┼─────────┤
[1] │ Bytes/Sec │ 7.17 MB │ 7.17 MB │ 8.4 MB │ 8.9 MB │ 8.37 MB  │ 384 kB   │ 7.17 MB │
[1] └───────────┴─────────┴─────────┴────────┴────────┴──────────┴──────────┴─────────┘
[1]
[1] Req/Bytes counts sampled once per second.
[1] # of samples: 30
[1]
[1] 1337k requests in 30.05s, 251 MB read
[1] autocannon -c 100 -d 30 -p 10 localhost:3000/ exited with code 0

Main Branch

> fastify@4.25.2 benchmark
> concurrently -k -s first "node ./examples/benchmark/simple.js" "autocannon -c 100 -d 30 -p 10 localhost:3000/"

[1] Running 30s test @ http://localhost:3000/
[1] 100 connections with 10 pipelining factor
[1]
[1] 
[1] ┌─────────┬──────┬───────┬───────┬────────┬──────────┬──────────┬────────┐
[1] │ Stat    │ 2.5% │ 50%   │ 97.5% │ 99%    │ Avg      │ Stdev    │ Max    │
[1] ├─────────┼──────┼───────┼───────┼────────┼──────────┼──────────┼────────┤
[1] │ Latency │ 8 ms │ 18 ms │ 95 ms │ 137 ms │ 21.96 ms │ 20.61 ms │ 213 ms │
[1] └─────────┴──────┴───────┴───────┴────────┴──────────┴──────────┴────────┘
[1] ┌───────────┬────────┬────────┬─────────┬─────────┬───────────┬─────────┬────────┐
[1] │ Stat      │ 1%     │ 2.5%   │ 50%     │ 97.5%   │ Avg       │ Stdev   │ Min    │
[1] ├───────────┼────────┼────────┼─────────┼─────────┼───────────┼─────────┼────────┤
[1] │ Req/Sec   │ 36,703 │ 36,703 │ 44,511  │ 46,975  │ 44,526.94 │ 1,874.7 │ 36,690 │
[1] ├───────────┼────────┼────────┼─────────┼─────────┼───────────┼─────────┼────────┤
[1] │ Bytes/Sec │ 6.9 MB │ 6.9 MB │ 8.37 MB │ 8.83 MB │ 8.37 MB   │ 352 kB  │ 6.9 MB │
[1] └───────────┴────────┴────────┴─────────┴─────────┴───────────┴─────────┴────────┘
[1] 
[1] Req/Bytes counts sampled once per second.
[1] # of samples: 30
[1] 
[1] 1337k requests in 30.04s, 251 MB read
[1] autocannon -c 100 -d 30 -p 10 localhost:3000/ exited with code 0

climba03003 · 2024-01-25T12:23:06Z

test/web-api.test.js

+  t.same(response.json(), { ok: true })
+})
+
+test('allow to pipe with undici.fetch', async (t) => {


@gurgunday
It is why I use Object.prototype.toString.call(payload) === '[object Response]'.

Uzlopak · 2024-01-25T12:34:42Z

lib/reply.js

+      typeof payload.getReader === 'function' ||
+      // Response
+      toString.call(payload) === '[object Response]'


Can we not create a symbol kResponse and set it as an attribute of the payload once, so we dont need to call toString multiple times?

I don't think we can skip it in .send but we can reduce one time in onSendEnd.
Note that due to the nature of Response, it is an immutable object. Which means every mutation (like status or headers) on it requires to re-create a new Response. Setting an attribute on it will be useless somehow.

mcollina

lgtm

lib/reply.js

test/web-api.test.js

Uzlopak · 2024-01-27T05:01:24Z

It feels strange that this PR should not have any performance penalty. The toString call is usually slowing down.

If the Response object is immutable, what if we store the result of toString.call(payload) === '[object Response]' as an attribute kIsResponse on our Reply Instance?

climba03003 · 2024-01-27T07:08:07Z

It feels strange that this PR should not have any performance penalty. The toString call is usually slowing down.

Do you have any reference on the toString call performance?
toString call is actually finding the Symbol.toStringTag in the object. It would be as fast as an property access if the implementation of class explicitly provide the symbol.

If the Response object is immutable, what if we store the result of toString.call(payload) === '[object Response]' as an attribute kIsResponse on our Reply Instance?

It is not an option because you would need to toString call anyway, this PR allows to change the payload to in any state which means it is not restricted to only .send API.
You need such identification of payload in .send or after onSend hook. The behavior is actually match the current implementation without an extra attribute.

Uzlopak

LGTM

github-actions · 2025-01-29T00:27:18Z

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.

Uzlopak mentioned this pull request Jan 25, 2024

feat: handle webstreams in reply.send #5280

Closed

4 tasks

gurgunday reviewed Jan 25, 2024

View reviewed changes

lib/reply.js Outdated Show resolved Hide resolved

mcollina reviewed Jan 25, 2024

View reviewed changes

lib/reply.js Outdated Show resolved Hide resolved

metcoder95 reviewed Jan 25, 2024

View reviewed changes

lib/reply.js Outdated Show resolved Hide resolved

climba03003 commented Jan 25, 2024

View reviewed changes

Uzlopak reviewed Jan 25, 2024

View reviewed changes

mcollina approved these changes Jan 25, 2024

View reviewed changes

Uzlopak added benchmark Label to run benchmark against PR and main branch and removed benchmark Label to run benchmark against PR and main branch labels Jan 25, 2024

climba03003 force-pushed the response branch from ff47a19 to a601e1c Compare January 25, 2024 17:18

mcollina requested review from Eomm, Uzlopak and jsumners January 26, 2024 17:51

Eomm reviewed Jan 26, 2024

View reviewed changes

lib/reply.js Outdated Show resolved Hide resolved

test/web-api.test.js Show resolved Hide resolved

climba03003 added 10 commits January 27, 2024 12:53

feat: Web Stream API

f0d34a6

chore: update error PR number

398517b

test: skip test when not available

078fd90

test: exit when skip test

ebd91c8

test: use Readable.toWeb instead

2093816

docs: update ToC and onSend hook

b005e8a

refactor: toString

9ecbb6b

refactor: direct for-of payload.headers

8934912

test: ensure compatibility of third-party Response

e4a537a

refactor: reduce toString call

833f0c5

climba03003 force-pushed the response branch from a601e1c to 833f0c5 Compare January 27, 2024 04:56

Eomm approved these changes Jan 27, 2024

View reviewed changes

metcoder95 approved these changes Jan 28, 2024

View reviewed changes

Uzlopak approved these changes Jan 28, 2024

View reviewed changes

mcollina merged commit 101ba57 into main Jan 29, 2024

mcollina deleted the response branch January 29, 2024 09:06

climba03003 mentioned this pull request Feb 2, 2024

http: split set-cookie when using setHeaders nodejs/node#51649

Merged

jadenPete mentioned this pull request Jan 2, 2025

Sending the same JavaScript Response twice can crash the server #5919

Closed

2 tasks

github-actions bot locked as resolved and limited conversation to collaborators Jan 29, 2025

Uh oh!

Conversation

climba03003 commented Jan 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

climba03003 commented Jan 25, 2024

Uh oh!

climba03003 Jan 25, 2024

Choose a reason for hiding this comment

Uh oh!

Uzlopak Jan 25, 2024

Choose a reason for hiding this comment

Uh oh!

climba03003 Jan 25, 2024 • 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!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uzlopak commented Jan 27, 2024

Uh oh!

climba03003 commented Jan 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uzlopak left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 29, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

climba03003 commented Jan 25, 2024 •

edited

Loading

climba03003 Jan 25, 2024 •

edited

Loading

climba03003 commented Jan 27, 2024 •

edited

Loading