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

zlib: expose amount of data read for engines #13088

Closed
wants to merge 5 commits into from

Conversation

AlexanderOMara
Copy link
Contributor

@AlexanderOMara AlexanderOMara commented May 18, 2017

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.

@nodejs-github-bot nodejs-github-bot added the zlib Issues and PRs related to the zlib subsystem. label May 18, 2017
@mscdex mscdex added the semver-minor PRs that contain new features and should be released in the next minor version. label May 18, 2017
Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

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

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 AlexanderOMara changed the title zlib: expose amount of data read for compression/decompression zlib: expose amount of data read for engines May 18, 2017
@AlexanderOMara
Copy link
Contributor Author

AlexanderOMara commented May 18, 2017

@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
Copy link
Member

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.

@AlexanderOMara
Copy link
Contributor Author

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

@@ -385,6 +385,12 @@ added: v0.5.8
Not exported by the `zlib` module. It is documented here because it is the base
class of the compressor/decompressor classes.

### zlib.bytesRead
Copy link
Member

Choose a reason for hiding this comment

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

Can you add

<!-- YAML
added: REPLACEME
-->

directly below this heading?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

doc/api/zlib.md Outdated

* {number}

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

@sam-github sam-github May 23, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@sam-github
Copy link
Contributor

Thanks @AlexanderOMara, it looks good.

@addaleax
Copy link
Member

@addaleax
Copy link
Member

Landed in e710036, thanks for the PR!

@addaleax addaleax closed this May 31, 2017
addaleax pushed a commit that referenced this pull request May 31, 2017
Added bytesRead property to Zlib engines

Fixes: #8874
PR-URL: #13088
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
jasnell pushed a commit that referenced this pull request Jun 5, 2017
Added bytesRead property to Zlib engines

Fixes: #8874
PR-URL: #13088
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
@jasnell jasnell mentioned this pull request Jun 5, 2017
@gibfahn
Copy link
Member

gibfahn commented Jan 15, 2018

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.

addaleax added a commit to addaleax/node that referenced this pull request Mar 29, 2018
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
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Apr 9, 2018
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 <matteo.collina@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Apr 12, 2018
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 <matteo.collina@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 1, 2018
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 <matteo.collina@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor PRs that contain new features and should be released in the next minor version. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants