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

Failing build since upgrading to v2.0.3 (Z_SYNC_FLUSH) #234

Closed
abarke opened this issue Jul 26, 2021 · 9 comments
Closed

Failing build since upgrading to v2.0.3 (Z_SYNC_FLUSH) #234

abarke opened this issue Jul 26, 2021 · 9 comments

Comments

@abarke
Copy link

abarke commented Jul 26, 2021

We recently upgraded pako from v1.011 to v2.0.3

This has broken our tests and build. I thought it was just the breaking constants, however it seems something else has also broken. The deflator now returns undefined when using Z_SYNC_FLUSH:

Pako v1.0.11

/**
 * Pako v1.0.11
 */
const pako = require('pako');

const deflator =  new pako.Deflate();

deflator.push('Test', pako.Z_SYNC_FLUSH);

if (deflator.err) { throw new Error(deflator.msg) }

console.log(deflator.result) // Uint8Array(12) [120, 156, 10, 73, 45, 46, 1, 0, 0, 0, 255, 255]

Pako v2.0.3

/**
 * Pako v2.0.3
 */
const pako = require('pako'); // v2.0.3

const deflator =  new pako.Deflate();

deflator.push('Test', pako.constants.Z_SYNC_FLUSH);

if (deflator.err) { throw new Error(deflator.msg) }

console.log(deflator.result) // undefined

Is this a bug or could you provide a suggestion?

@puzrin
Copy link
Member

puzrin commented Jul 26, 2021

Pako 2.x has rewritten top level loop, more close to zlib doc. I'd suggest to inspect this part https://github.com/nodeca/pako/blob/master/lib/deflate.js#L200-L267.

In worst case, you could create your own wrapper (that's plan B)

@puzrin
Copy link
Member

puzrin commented Jul 26, 2021

Also see 4e8ff8e

Seems Z_SYNC_FLUSH was dropped in process of wrapper rewrite - it was not clear how to do it, and how to test.

@abarke
Copy link
Author

abarke commented Jul 26, 2021

That's super unfortunate. We rely on this for rolling compression over WebSockets.

Could you re-implement this pls? Seems I'm not the only person with the same issue.

Otherwise what is the wrapper solution? Can I access the chunks before a flush occurs?

@puzrin
Copy link
Member

puzrin commented Jul 26, 2021

https://github.com/nodeca/pako/blob/master/lib/deflate.js - this is wrapper around ported zlib code. It's quite small. But it's not trivial to make it universal for all possible use cases.

  1. You can make PR with your cases + tests (then that will be maintainable and can be accepted)
  2. You can replace wrapper to one from v1
  3. You can stay at v1, because version bump was caused by wrapper rewrite. Low lever (zlib port) was not changed.

@abarke
Copy link
Author

abarke commented Jul 26, 2021

Many thanks for your rapid support 👍

@abarke
Copy link
Author

abarke commented Jul 26, 2021

I would like to contribute and write tests for this functionality, but time and knowledge of this lib is a boundary unfortunately. I hope this is re-introduced at some point so we can keep our WebSocket lib up-to-date. Thanks for a great lib 👏 its just a shame that there is now a wheel missing that used to be there 😄

@puzrin
Copy link
Member

puzrin commented Jul 26, 2021

You can try to propose tests only (disabled by default with it.skip(..)). Problem with Z_SYNC_FLUSH in v1 - it was PR-ed without tests => that was not maintainable. So, i could not port this support to v2.

@puzrin
Copy link
Member

puzrin commented Jul 26, 2021

https://github.com/nodeca/pako/blob/master/lib/deflate.js#L258 as far as i see, wrapper should flush buffers as expected, if you pass Z_SYNC_FLUSH (flush > 0).

So, you can extract deflated data from .chunks[].

@puzrin
Copy link
Member

puzrin commented Jul 29, 2021

Closing, see #234 (comment)

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

No branches or pull requests

2 participants