zlib: inconsistent call to flush callback on oversized buffers

[jasnell]

When using an oversized buffer, the flush callback is only sometimes called.

'use strict';
const zlib = require('zlib');

const buf = new Buffer(100000);
const def = zlib.createDeflate({
  highWaterMark:5,
  level: 2
});
def.on('drain', ()=> console.log('drained'));
def.write(buf, ()=> {
  console.log('after write');
  def.flush(zlib.Z_FULL_FLUSH, function(err) {
    console.log('flushed');
  });
  def.end();
});

bash-3.2$ ./node ~/test.js
drained
after write
bash-3.2$ ./node ~/test.js
drained
after write
flushed
bash-3.2$ 

Relevant IRC Chat

15:46 jasnell: not yet... the inconsistency seems to have something to do with the size of the buffer
15:46 jasnell: if I drop down to Buffer(10000), it works fine
15:46 chrisdickinson: that's < zlib window size
15:47 chrisdickinson: it might be in limbo waiting for a corresponding read
15:47 thealphanerd: adding a read does give a consisent flush
15:48 jasnell: ok, makes for a rather tricky inconsistency
15:48 jasnell: sometimes it works, sometimes it doesn't, with no indication as to why
15:50 thealphanerd: jasnell: I think you can check to see if write returns false?
15:51 chrisdickinson: ah
15:51 chrisdickinson: might see how many times the handle is written to per-buffer
15:51 chrisdickinson: https://github.com/nodejs/node/blob/v4.2.1/lib/zlib.js#L588-L593

Will be getting back to this to investigate. @chrisdickinson @thealphanerd

Related to: #3534

[jasnell]

Further discussion in IRC points to: when passing a large amount of data into zlib, the readable state's highwatermark may max out. When this happens, the writable buffer cannot fully flush until the readable state is cleared.

[jasnell]

The fix for this is to improve the documentation and provide an example of dealing with large amounts of compressed data.

[MylesBorins]

@chrisdickinson / @jasnell please correct me if I am wrong, but does this not apply as a general case in Duplex / transform streams? If so should this be documented in Zlib or in the general stream documentation?

[chrisdickinson]

@thealphanerd I'd lean towards noting this in the zlib docs. .flush (as opposed to ._flush) is specific to zlib streams, and doesn't quite do what it says on the tin — it queues a flush (potentially behind many pending writes) instead of immediately flushing.

[jasnell]

@nodejs/documentation

[Knighton910]

zlib docs

But this is noting on that isn't it?

[addaleax]

@lordKnighton Not really… the current docs only refer to how .flush() affects the compression quality.

This is actually not very zlib-specific – the same mechanism is at work in code like this:

const stream = require('stream');
const s = new stream.PassThrough({
  highWaterMark: 5
});
s.write('Hello, World!', () => console.log('Wrote first chunk'));
s.write('', () => console.log('Wrote empty chunk'));

The second callback will not be invoked because the (Readable’s) highWaterMark effects exactly what it is supposed to, namely indicating that the stream does not want to produce more output when a given number of bytes has been buffered.

The only thing that’s a little specific to zlib here is the choice of the method name flush, as it is commonly associated with doing something more or less immediately. I don’t really know what a good addition to the docs could look like, but honestly, I think the description of highWaterMark as The maximum number of bytes to store in the internal buffer before ceasing to read from the underlying resource actually does pretty well here on its own…

[addaleax]

What @chrisdickinson was referring to was that flush takes effect only after the current set of pending writes has been processed (i.e. on drain). That may actually be worth noting in the documentation? Either way that is actually not related to the original issue here, where there are no pending writes.

[Knighton910]

Nice response, 👍 for the explanation.