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.

Sign up for GitHub

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

Jump to bottom

http: add bytesWritten property #36964

Closed
wants to merge 8 commits into from
Closed

http: add bytesWritten property #36964

wants to merge 8 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Jan 16, 2021

Adds a bytesWritten property which can be used to inspect
how many bytes have been dispatched to response body.

@ronag
http: add bytesWritten property
5b40b59 
Adds a bytesWritten property which can be used to inspect
how many bytes have been dispatched to response body.
@ronag ronag added http Issues or PRs related to the http subsystem. semver-minor PRs that contain new features and should be released in the next minor version. http2 Issues or PRs related to the http2 subsystem. labels Jan 16, 2021
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jan 16, 2021
@ronag
Copy link
Member Author

ronag commented Jan 16, 2021

@nodejs/http @nodejs/http2

@ronag
Copy link
Member Author

ronag commented Jan 16, 2021

This is quite commonly implemented in user land to different degrees of correctness.

RaisinTen
RaisinTen reviewed Jan 16, 2021
View reviewed changes
doc/api/http.md Outdated Show resolved Hide resolved
@ronag @RaisinTen
Update doc/api/http.md
ebd4877 
Co-authored-by: Darshan Sen <raisinten@gmail.com>
ronag
ronag commented Jan 16, 2021
View reviewed changes
doc/api/http2.md Outdated Show resolved Hide resolved
ronag
ronag commented Jan 16, 2021
View reviewed changes
lib/_http_outgoing.js
get() {
return this[kBytesWritten];
}
});
Copy link
Member Author

Choose a reason for hiding this comment

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

Could this break userland which extend e.g. ServerResponse and add this themselves?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure. Maybe some web framework listed in https://github.com/nodejs/citgm/blob/main/lib/lookup.json? I think we could also try some HTTP clients like got.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a setter as well to try and avoid breaking

aduh95
aduh95 reviewed Jan 16, 2021
View reviewed changes
lib/internal/http2/core.js Outdated Show resolved Hide resolved
lib/internal/http2/core.js Outdated Show resolved Hide resolved
ronag and others added 3 commits January 16, 2021 16:07
@ronag @aduh95
Update lib/internal/http2/core.js
6a894b3 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@ronag
fixup
f54be73
@ronag
fixup
7733bdd
@ronag ronag added the needs-citgm PRs that need a CITGM CI run. label Jan 16, 2021
@lpinca
Copy link
Member

lpinca commented Jan 16, 2021

Does this have a noticeable performance impact?

@ronag
Copy link
Member Author

ronag commented Jan 16, 2021 •
edited
Loading

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/899/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2567/

mcollina
mcollina requested changes Jan 16, 2021
View reviewed changes
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.

My experience with Buffer.byteLength is that it slowish.

I don't necessarily see this as something worth the penalty (if any).

@ronag
Copy link
Member Author

ronag commented Jan 16, 2021

@mcollina: How about now? This should be negligable.

mcollina
mcollina requested changes Jan 16, 2021
View reviewed changes
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.

I fear this would even be worse :/

@ronag ronag closed this Jan 16, 2021
@ronag
Copy link
Member Author

ronag commented Jan 16, 2021

I can resolve this by overriding ServerResponse and doing it myself in user land. However, there is no such option for http2 unfortunately :/.

@ronag ronag reopened this Jan 16, 2021
@ronag
Copy link
Member Author

ronag commented Jan 16, 2021

Ah, I can solve this in userland using options.Http2ServerResponse.

@ronag ronag closed this Jan 16, 2021
@lpinca lpinca mentioned this pull request Mar 10, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 left review comments

@dnlup dnlup dnlup left review comments

@RaisinTen RaisinTen RaisinTen left review comments

@mcollina mcollina mcollina requested changes

@tniessen tniessen Awaiting requested review from tniessen

@ryzokuken ryzokuken Awaiting requested review from ryzokuken

@lundibundi lundibundi Awaiting requested review from lundibundi

@lpinca lpinca Awaiting requested review from lpinca

Assignees
No one assigned
Labels
http Issues or PRs related to the http subsystem. http2 Issues or PRs related to the http2 subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. needs-citgm PRs that need a CITGM CI run. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ronag @lpinca @mcollina @aduh95 @dnlup @RaisinTen @nodejs-github-bot