[x] http: bytesWritten for http/1 and compat #28004

ronag · 2019-06-01T17:48:31Z

Adds bytesWritten helper to http/1 & compat response.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

addaleax

LGTM with 1 request

lib/internal/http2/core.js

ronag · 2019-06-02T17:44:21Z

Would be nice to add bytesRead to IncomingMessage... but I can't follow the data flow there...

BridgeAR

This seems to require some documentation?

BridgeAR · 2019-07-05T01:02:46Z

Ping @ronag

ronag · 2019-07-13T11:51:05Z

@BridgeAR docs added

Trott · 2019-07-14T08:43:39Z

@nodejs/http @nodejs/http2

mcollina

I don't think this is compatible with the approach in #27988. If we think we should add bytesRead to Readable, we should add bytesWritten to Writable.

ronag · 2019-07-14T11:03:05Z

@mcollina What do you mean? http/1 does not inherit from Readable nor Writable. Or do you mean you are missing the bytesRead?

mcollina · 2019-07-14T11:08:27Z

@ronag IncomigMessage inherits from Readable (source

node/lib/_http_incoming.js

Line 77 in e12f48e

Object.setPrototypeOf(IncomingMessage, Stream.Readable);

). OutgoingMessage does not (but it quacks like it).

I'm -1 in adding bytesWritten here if it's not part of Readable/Writable as well.

ronag · 2019-07-14T11:21:32Z

@mcollina I've updated the PR so that the classes that inherit Readable/Writable do not get these props overloaded.

However, OutgoingMessage is supposed to "quack" like a Writable so I still think it's appropriate to add this property (at least until we get so far that we can implemented it through Writable)

ronag · 2019-07-27T12:18:39Z

Should I give up and close this one?

ronag · 2019-07-27T12:24:21Z

not sure, should this be the amount sent to write or the amount that has actually been flushed? @mcollina?

mcollina · 2019-08-02T06:55:00Z

I think we are computing it in the right moment. Bytes written to the stream, not bytes flushed out.

ronag · 2019-08-02T07:06:24Z

@mcollina: What is left here in order to make this mergable?

mcollina · 2019-08-02T07:14:34Z

Unit tests.

I’m also unsure about the naming.
What happens to this value if the stream is in object mode? Should it be “dataWritten” instead?

ronag · 2019-08-02T07:23:42Z

Unit tests.

Ok.

I’m also unsure about the naming.
What happens to this value if the stream is in object mode? Should it be “dataWritten” instead?

Yes I was unsure about this myself. What about just written (that's what we call it on the stream state)? dataWritten doesn't feel right to me at least.

mcollina · 2019-08-02T07:44:04Z

I would go for writableWritten, even if it sounds bad: written might be too generic and conflict with some other properties.

ronag · 2019-08-02T07:47:34Z

Tests added

mcollina · 2019-08-03T10:04:34Z

@addaleax @lpinca how would you call the amount of data (bytes or object) written to a Writable? My best option is writableWritten, which I dislike.

addaleax · 2019-08-03T13:05:38Z

@mcollina I’d really prefer to keep consistency with the existing properties that provide this information, which are called bytesRead and bytesWritten. That might be unfortunate but I think it’s the most consistent and least confusing.

mcollina · 2019-08-03T16:01:41Z

I think we should document and test the behavior of these properties with objectMode: true.

lpinca · 2019-08-03T16:44:18Z

I kind of agree with @addaleax about names.

ronag · 2019-08-24T10:27:07Z

Rebased, cleaned up and fixed test.

What's the consensus in regards to:

The naming?
should bytesWritten represent the number of objects written in objectMode or should it be null.

ronag · 2019-08-24T10:33:45Z

This might need a semver-major. We already have bytesWritten on fs streams and there it is/was slightly different/wrong.

ronag · 2019-08-24T10:36:49Z

doc/api/fs.md

+
+Integer value that indicates how many bytes has been written to
+the response body (not the amount flushed).



Do we even need this here since WriteStream inherits from Writable which now documents this property? Remove?

ronag · 2019-08-24T10:38:26Z

lib/internal/fs/streams.js

      }
      return cb(er);
    }
-    this.bytesWritten += bytes;


notice that WriteStream increments bytesWritten inside _write() callback and not in write(). I think this is actually wrong and slightly contradicts the previous documentation as well (and possibly expected behaviour, it surprised me).

ronag · 2019-08-24T10:38:32Z

lib/internal/fs/streams.js

      self.destroy();
      return cb(er);
    }
-    self.bytesWritten += bytes;


See comment above.

ronag · 2019-08-26T13:17:38Z

I'm closing this for now.

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jun 1, 2019

addaleax reviewed Jun 2, 2019

View reviewed changes

lib/internal/http2/core.js Outdated Show resolved Hide resolved

addaleax added http Issues or PRs related to the http subsystem. http2 Issues or PRs related to the http2 subsystem. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Jun 2, 2019

ronag force-pushed the feat-http1-compat-bytesWritten branch 2 times, most recently from 683804b to 8720c1c Compare June 2, 2019 17:39

BridgeAR reviewed Jun 4, 2019

View reviewed changes

BridgeAR added the semver-minor PRs that contain new features and should be released in the next minor version. label Jun 4, 2019

ronag force-pushed the feat-http1-compat-bytesWritten branch 2 times, most recently from b627fb5 to 5d10298 Compare July 13, 2019 11:51

ronag force-pushed the feat-http1-compat-bytesWritten branch from 5d10298 to c5cfabf Compare July 13, 2019 11:51

mcollina requested changes Jul 14, 2019

View reviewed changes

ronag force-pushed the feat-http1-compat-bytesWritten branch from c5cfabf to 7550220 Compare July 14, 2019 11:20

ronag force-pushed the feat-http1-compat-bytesWritten branch 2 times, most recently from ef314da to d322bc4 Compare July 27, 2019 12:23

ronag force-pushed the feat-http1-compat-bytesWritten branch from d322bc4 to 86ee44a Compare August 2, 2019 07:40

ronag force-pushed the feat-http1-compat-bytesWritten branch from 86ee44a to dccba4d Compare August 2, 2019 07:47

ronag force-pushed the feat-http1-compat-bytesWritten branch 4 times, most recently from 13157ec to ed38fa8 Compare August 2, 2019 09:17

ronag force-pushed the feat-http1-compat-bytesWritten branch from ed38fa8 to 499c736 Compare August 24, 2019 10:26

ronag mentioned this pull request Aug 24, 2019

[x] stream: add bytesRead to Readable #27988

Closed

4 tasks

ronag force-pushed the feat-http1-compat-bytesWritten branch 4 times, most recently from 0f6a4e8 to 3b07f24 Compare August 24, 2019 10:33

http: bytesWritten for http/1

4a01aa8

ronag force-pushed the feat-http1-compat-bytesWritten branch from 3b07f24 to 4a01aa8 Compare August 24, 2019 10:35

ronag commented Aug 24, 2019

View reviewed changes

lib/internal/fs/streams.js

self.destroy();

return cb(er);

}

self.bytesWritten += bytes;

Copy link

Member Author

ronag Aug 24, 2019

Choose a reason for hiding this comment

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

See comment above.

ronag closed this Aug 26, 2019

ronag changed the title ~~http: bytesWritten for http/1 and compat~~ [x] http: bytesWritten for http/1 and compat Aug 26, 2019


		Integer value that indicates how many bytes has been written to
		the response body (not the amount flushed).

Uh oh!

[x] http: bytesWritten for http/1 and compat #28004

[x] http: bytesWritten for http/1 and compat #28004

Uh oh!

Conversation

ronag commented Jun 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ronag commented Jun 2, 2019

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Jul 5, 2019

Uh oh!

ronag commented Jul 13, 2019

Uh oh!

Trott commented Jul 14, 2019

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

ronag commented Jul 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Jul 14, 2019

Uh oh!

ronag commented Jul 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Jul 27, 2019

Uh oh!

ronag commented Jul 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Aug 2, 2019

Uh oh!

ronag commented Aug 2, 2019

Uh oh!

mcollina commented Aug 2, 2019

Uh oh!

ronag commented Aug 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Aug 2, 2019

Uh oh!

ronag commented Aug 2, 2019

Uh oh!

mcollina commented Aug 3, 2019

Uh oh!

addaleax commented Aug 3, 2019

Uh oh!

mcollina commented Aug 3, 2019

Uh oh!

lpinca commented Aug 3, 2019

Uh oh!

ronag commented Aug 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Aug 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag Aug 24, 2019

Choose a reason for hiding this comment

Uh oh!

ronag Aug 24, 2019

Choose a reason for hiding this comment

Uh oh!

ronag Aug 24, 2019

Choose a reason for hiding this comment

Uh oh!

ronag commented Aug 26, 2019

Uh oh!

Reviewers

Assignees

ronag commented Jun 1, 2019 •

edited

Loading

ronag commented Jul 14, 2019 •

edited

Loading

ronag commented Jul 14, 2019 •

edited

Loading

ronag commented Jul 27, 2019 •

edited

Loading

ronag commented Aug 2, 2019 •

edited

Loading

ronag commented Aug 24, 2019 •

edited

Loading

ronag commented Aug 24, 2019 •

edited

Loading