zlib: expose amount of data read for engines#13088

AlexanderOMara · 2017-05-18T00:53:15Z

Added bytesRead property to Zlib engines

Refs: #8874

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

Affected core subsystem(s)

zlib

Alternative to Pull Request #12939, broken into 2 separate commits.

sam-github

Thanks, particularly for the test case. You'll need to add documentation before this can land, and shorten the commit message to 50 characters.

Added bytesRead property to Zlib engines Refs: nodejs#8874

AlexanderOMara · 2017-05-18T03:56:39Z

@sam-github I've fixed the commit message, and I'll work on the documentation.

I have one question though:

What is the purpose of the reset method? I'm trying to determine if calling that method should reset the bytesRead property to 0.

addaleax · 2017-05-18T14:37:45Z

What is the purpose of the reset method? I'm trying to determine if calling that method should reset the bytesRead property to 0.

I think the public method was actually supposed to enable re-use of zlib contexts, probably for efficiency. It was added in 71ae175, but that was a long time ago and I’m not sure if @indutny remembers more details. 😄

Internally, the method is used for e.g. resetting after a member of a gzip file containing multiple members has been decompressed (echo 'bar' | gzip > foo.gz; echo 'baz' | gzip >> foo.gz; gunzip < foo.gz).

I think it’s okay to leave it alone and not reset the counter.

Refs: nodejs#8874

AlexanderOMara · 2017-05-22T03:40:37Z

@sam-github I've added some documentation. Let me know if it needs any changes.

addaleax · 2017-05-23T18:05:20Z

doc/api/zlib.md

 Not exported by the `zlib` module. It is documented here because it is the base
 class of the compressor/decompressor classes.

+### zlib.bytesRead


Can you add

directly below this heading?

addaleax · 2017-05-23T18:07:55Z

test/parallel/test-zlib-bytes-read.js

+constwrite=()=>{
+target.write(Buffer.from([buffer[writer.size++]]));
+if(writer.size<buffer.length){
+setTimeout(write,25);


This looks a bit odd… I think what you want is to do something once the write finished, but the right way to do that would be to pass a callback to target.write(), instead of relying on timeouts

Ah, I didn't realize there were callbacks for these methods. That's a much better idea.

sam-github · 2017-05-23T18:23:18Z

doc/api/zlib.md

+
+*{number}
+
+The `zlib.bytesRead` property specifies the number of bytes read by the engine.


bytes in -> zlib engine -> bytes out: this is the bytes read by the engine from the encrypted stream (bytes out), or the other side (bytes in)? I suspect its bytes in, but it should be overwhelmingly clear from the docs, please, perhaps "bytes read by the engine before the bytes are processed (compressed or decompressed, as appropriate for the derived class)" <--- or change the language if its bytes read post processing.

Yes, it's the bytes in. Description expanded.

Refactored bytesRead test to use callbacks instead of timers Refs: nodejs#8874

Refs: nodejs#8874

sam-github · 2017-05-24T04:43:55Z

Thanks @AlexanderOMara, it looks good.

addaleax · 2017-05-24T10:47:34Z

CI: https://ci.nodejs.org/job/node-test-commit/10119/

addaleax · 2017-05-31T22:25:36Z

Landed in e710036, thanks for the PR!

Added bytesRead property to Zlib engines Fixes: #8874 PR-URL: #13088 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Sam Roberts <[email protected]>

gibfahn · 2018-01-15T21:44:32Z

Release team decided not to backport to v6.x, if you disagree let us know.

@addaleax if you think this should go back to v6.x and would be willing to backport, tell us.

The introduction of `.bytesRead` to zlib streams was unfortunate, because other streams in Node.js core use the exact opposite naming of `.bytesRead` and `.bytesWritten`. While one could see how the original naming makes sense in a `Transform` stream context, we should try to work towards more consistent APIs in core for these things. This introduces `zlib.bytesWritten` and documentation-only deprecates `zlib.bytesRead`. (Luckily, the old property name hasn’t even been around for a whole year yet.) Refs: nodejs#8874 Refs: nodejs#13088

The introduction of `.bytesRead` to zlib streams was unfortunate, because other streams in Node.js core use the exact opposite naming of `.bytesRead` and `.bytesWritten`. While one could see how the original naming makes sense in a `Transform` stream context, we should try to work towards more consistent APIs in core for these things. This introduces `zlib.bytesWritten` and documentation-only deprecates `zlib.bytesRead`. PR-URL: nodejs#19414 Refs: nodejs#8874 Refs: nodejs#13088 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

The introduction of `.bytesRead` to zlib streams was unfortunate, because other streams in Node.js core use the exact opposite naming of `.bytesRead` and `.bytesWritten`. While one could see how the original naming makes sense in a `Transform` stream context, we should try to work towards more consistent APIs in core for these things. This introduces `zlib.bytesWritten` and documentation-only deprecates `zlib.bytesRead`. PR-URL: #19414 Refs: #8874 Refs: #13088 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

The introduction of `.bytesRead` to zlib streams was unfortunate, because other streams in Node.js core use the exact opposite naming of `.bytesRead` and `.bytesWritten`. While one could see how the original naming makes sense in a `Transform` stream context, we should try to work towards more consistent APIs in core for these things. This introduces `zlib.bytesWritten` and documentation-only deprecates `zlib.bytesRead`. PR-URL: nodejs#19414 Refs: nodejs#8874 Refs: nodejs#13088 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

nodejs-github-bot added the zlib Issues and PRs related to the zlib subsystem. label May 18, 2017

mscdex added the semver-minor PRs that contain new features and should be released in the next minor version. label May 18, 2017

AlexanderOMara mentioned this pull request May 18, 2017
zlib: expose amount of data read for compression/decompression #12939
Closed
4 tasks

sam-github suggested changes May 18, 2017
View reviewed changes

zlib: expose amount of data read for engines
7aa6e2b
Added bytesRead property to Zlib engines Refs: nodejs#8874

AlexanderOMara force-pushed the zlib-bytes-read branch from fc8498f to 7aa6e2bCompare May 18, 2017 01:10

AlexanderOMara changed the title ~~zlib: expose amount of data read for compression/decompression~~zlib: expose amount of data read for enginesMay 18, 2017

zlib: document engine bytesRead property
df6c1f0
Refs: nodejs#8874

addaleax reviewed May 23, 2017
View reviewed changes

sam-github suggested changes May 23, 2017
View reviewed changes

AlexanderOMara added 3 commits May 23, 2017 23:14

zlib: refactored bytesRead test to use callbacks
cf58570
Refactored bytesRead test to use callbacks instead of timers Refs: nodejs#8874

zlib: expanded bytesRead property description
f5a87d6
Refs: nodejs#8874

zlib: added bytesRead YAML comment
6c1a1e9
Refs: nodejs#8874

sam-github approved these changes May 24, 2017
View reviewed changes

addaleax approved these changes May 24, 2017
View reviewed changes

addaleax closed this May 31, 2017

addaleax pushed a commit that referenced this pull request May 31, 2017
zlib: expose amount of data read for engines
e710036
Added bytesRead property to Zlib engines Fixes: #8874 PR-URL: #13088 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Sam Roberts <[email protected]>

jasnell pushed a commit that referenced this pull request Jun 5, 2017
zlib: expose amount of data read for engines
cc3174a
Added bytesRead property to Zlib engines Fixes: #8874 PR-URL: #13088 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Sam Roberts <[email protected]>

jasnell mentioned this pull request Jun 5, 2017
8.1.0 Proposal #13483
Closed

gibfahn added dont-land-on-v6.x labels Jan 15, 2018


		*{number}

		The `zlib.bytesRead` property specifies the number of bytes read by the engine.

Uh oh!

zlib: expose amount of data read for engines#13088

zlib: expose amount of data read for engines #13088

Uh oh!

Conversation

AlexanderOMara commented May 18, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

sam-github left a comment

Choose a reason for hiding this comment

Uh oh!

AlexanderOMara commented May 18, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented May 18, 2017

Uh oh!

AlexanderOMara commented May 22, 2017

Uh oh!

addaleaxMay 23, 2017

Choose a reason for hiding this comment

Uh oh!

AlexanderOMaraMay 24, 2017

Choose a reason for hiding this comment

Uh oh!

addaleaxMay 23, 2017

Choose a reason for hiding this comment

Uh oh!

AlexanderOMaraMay 24, 2017

Choose a reason for hiding this comment

Uh oh!

sam-githubMay 23, 2017• 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!

AlexanderOMaraMay 24, 2017

Choose a reason for hiding this comment

Uh oh!

sam-github commented May 24, 2017

Uh oh!

addaleax commented May 24, 2017

Uh oh!

addaleax commented May 31, 2017

Uh oh!

gibfahn commented Jan 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

AlexanderOMara commented May 18, 2017•
edited
Loading

AlexanderOMara commented May 18, 2017•
edited
Loading

sam-githubMay 23, 2017•
edited
Loading