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

feat: add undici:body:consumed diagnostics channel #1369

Closed
wants to merge 1 commit into from
Closed

feat: add undici:body:consumed diagnostics channel #1369

wants to merge 1 commit into from

Conversation

KhafraDev
Copy link
Member

Fixes #1342

Superseded #1344 with a slightly more reliable solution (relies on consuming via the body mixins).

@codecov-commenter
Copy link

codecov-commenter commented Apr 24, 2022

Codecov Report

Merging #1369 (6741636) into main (ae369b2) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1369      +/-   ##
==========================================
+ Coverage   94.25%   94.26%   +0.01%     
==========================================
  Files          45       45              
  Lines        4211     4222      +11     
==========================================
+ Hits         3969     3980      +11     
  Misses        242      242              
Impacted Files Coverage Δ
lib/api/api-request.js 100.00% <100.00%> (ø)
lib/api/readable.js 89.76% <100.00%> (+0.97%) ⬆️
lib/client.js 97.93% <100.00%> (ø)
lib/core/request.js 98.65% <100.00%> (ø)
lib/handler/redirect.js 94.93% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae369b2...6741636. Read the comment docs.

Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

I'm sorry but I don't like this. Can't you just have a event for each body chunk? and then trailers will signal end of body?

@KhafraDev
Copy link
Member Author

KhafraDev commented Apr 24, 2022

I can look into it, but this is also opt-in (rather than the old PR where it was added onto the trailers channel), and it only sends the body that was already consumed, rather than duplicating it.

Also publishing a message for each chunk would make it very hard to group together correctly, since 2 requests to the same origin/path could be made at once.

edit: also in the code I used channels.trailers when it's not part of the trailers channel, but its own separate channel (whoops). If there are no subscribers to this channel, nothing is sent. I also don't think it would be bad for performance because it doesn't do anything under the hood, other than pass the body to the channel, which was already consumed by the user.

import { request } from 'undici'
import { text } from 'stream/consumers'

const { body } = await request('...') // nothing is published

await body.text() // a message is published, identical to the value this method returned

await text(body) // no message published

for await (const chunk of body) {
  // ...
}

// no message published

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm, good work

@KhafraDev
Copy link
Member Author

I'm getting mixed signals here 😅

@mcollina
Copy link
Member

I'm sorry but I don't like this. Can't you just have a event for each body chunk? and then trailers will signal end of body?

I have a feeling this could impact perf in the wrong way as that's a fairly hot path. Anyway if that does not regress the perf, it would be ok.

@KhafraDev
Copy link
Member Author

I think this way is more convenient and faster. It also only requires 1 message rather than x amount of chunks received and doesn't require the user to parse the body themselves.

This pull request was closed.
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.

[diagnostics_channel] support body received?
4 participants