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

Transform stream misses final readable event for small inputs #25810

Closed
sfriesel opened this issue Jan 30, 2019 · 14 comments
Closed

Transform stream misses final readable event for small inputs #25810

sfriesel opened this issue Jan 30, 2019 · 14 comments
Labels
confirmed-bug Issues with confirmed bugs. stream Issues and PRs related to the stream subsystem.

Comments

@sfriesel
Copy link

sfriesel commented Jan 30, 2019

  • Version: v11.8.0
  • Platform: Ubuntu 18.10
  • Subsystem: stream

A Transform stream that transforms the entire input in one shot will generate just one readable event, when it should be two (one for data and one for EOS).

const stream = require('stream')
const fs = require('fs');

const r = new stream.Readable();
r._read = function(n) { this.push('content'); this.push(null); };
// const r = fs.createReadStream('/boot/memtest86+.bin');

var t = new stream.Transform({
  transform: function(chunk, encoding, callback) {
    console.log('_transform');
    this.push(chunk);
    return void callback();
  },
  flush: function(callback) {
    console.log('_flush');
    return void callback();
  }
});

r.pipe(t);
t.on("readable", function() {
  console.log("on readable");
  while (true) {
    var chunk = t.read();
    console.log("chunk", chunk);
    if (!chunk)
      break;
  }
});

The output of this example is:

_transform
_flush
on readable
chunk <Buffer 63 6f 6e 74 65 6e 74>
chunk null

But it should be

_transform
_flush
on readable
chunk <Buffer 63 6f 6e 74 65 6e 74>
chunk null
on readable
chunk null

Using a larger input stream (like the commented out line) correctly produces the last readable event.

@addaleax addaleax added confirmed-bug Issues with confirmed bugs. stream Issues and PRs related to the stream subsystem. labels Jan 30, 2019
@addaleax
Copy link
Member

/cc @nodejs/streams

@starkwang
Copy link
Contributor

null is regarded as a EOF when readable.push(null).

function readableAddChunk(stream, chunk, encoding, addToFront, skipChunkCheck) {
debug('readableAddChunk', chunk);
var state = stream._readableState;
if (chunk === null) {
state.reading = false;
onEofChunk(stream, state);

@mcollina
Copy link
Member

I don't think this is a bug. This is working exactly as expected.

In the first case you are ending the stream synchronously, so there is only 1 readable event. In the second case, it's an asynchronous operation, so there are two 'readable' event. It might sound unintuitive, but this is minimum number of events.

@sfriesel
Copy link
Author

I don't think this is a bug. This is working exactly as expected.

Quoting the stream documentation:

The 'readable' event will also be emitted once the end of the stream data has been reached but before the 'end' event is emitted.
Effectively, the 'readable' event indicates that the stream has new information: either new data is available or the end of the stream has been reached. In the former case, stream.read() will return the available data. In the latter case, stream.read() will return null.

(emphasis added)
To my understanding, the documentation says the event will only notify about one of the two conditions at a time. The way for the readable handler to detect EOS is by checking the first result of read for null, which it can't if both events get delivered as one.
The documentation also doesn't mention that ending streams synchronously or asynchronously would make a difference.

@mcollina
Copy link
Member

I digged some more. This is a regression from Node 8.
I was put off by the comment in #25810 (comment), which was pointing to the expected behavior.

@mcollina
Copy link
Member

This is happening when another 'readable' event is already scheduled to execute, so we are not scheduling two anymore.

@sfriesel
Copy link
Author

Not sure whether related or not: when I replace the dummy Transform object above with zlib.createGzip() (which inherits from Transform), it never seems to generate the final readable event on EOS irrespective of how the input is provided.

@mcollina
Copy link
Member

mcollina commented Feb 9, 2019

This is odd, it seems some sort of unexpected bug/interaction with Transform. I'm looking into it.

@lpinca
Copy link
Member

lpinca commented Feb 13, 2019

This work as expected on Node.js 8 and 9 but not on 10 so we had a regression.

@lpinca
Copy link
Member

lpinca commented Feb 14, 2019

Bisecting points to this commit: 1e0f331

@mcollina
Copy link
Member

@lpinca I expected as much, and it was a failure of our unit test not covering this high level behaviour to begin with. Note that the test I'm breaking the most in #26059 was added in 1e0f331.

The fact that this emerged right now is means is not a bad regression.

mcollina added a commit to mcollina/node that referenced this issue Mar 4, 2019
@mcollina
Copy link
Member

mcollina commented Mar 6, 2019

Fixed in e95e7f9

@mcollina mcollina closed this as completed Mar 6, 2019
BridgeAR pushed a commit to BridgeAR/node that referenced this issue Mar 12, 2019
@mcollina
Copy link
Member

Reopening because of #26617. Sigh, my fix broke stuff.

@mcollina mcollina reopened this Mar 12, 2019
mcollina added a commit to mcollina/node that referenced this issue Mar 13, 2019
In nodejs#26059, we introduced a bug that caused 'readable' to be nextTicked
on EOF of a ReadableStream. This breaks the dicer module on CITGM.
That change was partially reverted to still fix the bug in nodejs#25810 and
not break dicer.

See: nodejs#26059
Fixes: nodejs#25810
@addaleax addaleax reopened this Mar 13, 2019
@mcollina
Copy link
Member

Fixed in 269103a

mcollina added a commit that referenced this issue Mar 16, 2019
In #26059, we introduced a bug that caused 'readable' to be nextTicked
on EOF of a ReadableStream. This breaks the dicer module on CITGM.
That change was partially reverted to still fix the bug in #25810 and
not break dicer.

See: #26059
Fixes: #25810

PR-URL: #26643
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit to targos/node that referenced this issue Mar 27, 2019
targos pushed a commit to targos/node that referenced this issue Mar 27, 2019
In nodejs#26059, we introduced a bug that caused 'readable' to be nextTicked
on EOF of a ReadableStream. This breaks the dicer module on CITGM.
That change was partially reverted to still fix the bug in nodejs#25810 and
not break dicer.

See: nodejs#26059
Fixes: nodejs#25810

PR-URL: nodejs#26643
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue Mar 27, 2019
targos pushed a commit that referenced this issue Mar 27, 2019
In #26059, we introduced a bug that caused 'readable' to be nextTicked
on EOF of a ReadableStream. This breaks the dicer module on CITGM.
That change was partially reverted to still fix the bug in #25810 and
not break dicer.

See: #26059
Fixes: #25810

PR-URL: #26643
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. stream Issues and PRs related to the stream subsystem.
Projects
None yet
5 participants