search

dominictarr / through

simple way to create a ReadableWritable stream that works
Other
670 stars 64 forks source link

no 'close' event on node 0.11.2 (ok on 0.11.0, 0.10.x, 0.8.x) #17

Closed isao closed 11 years ago

isao commented 11 years ago

I use through to do key/value replacement on a file stream. On node 0.11.2 the "close" event is never emitted. But things are fine on node versions 0.11.0, 0.8.x & 0.10.x.

The through-stream code is replacers.js. It is used here.

If I change those lines from 

read.pipe(replacer(this.data))
    .pipe(write);

to

read.pipe(write);

...the "close" event is emitted as expected.

isao commented 11 years ago

maybe related to stream changes? https://github.com/joyent/node/blob/master/ChangeLog

or maybe I'm doing something dumb...

dominictarr commented 11 years ago

if this has been introduced in 0.11, then possibly this is an issue with node 0.11, can you post an issue on node and link it here?

isao commented 11 years ago

I can open an issue on node, since AFAICT it worked up until 0.11.0, and only fails in 0.11.2. But since things work if I remove the through-stream part, I'm not sure they'll think it's a node issue.

I'll try to do a simplified repro case, and maybe I'll get some more insight either way...

Thanks for the response (and all the libraries).

dominictarr commented 11 years ago

streams2 is ment to be backwards compatible, so if this breaks old streams, then that is not backwards compatible.

hmm, maybe it's to do with the finish event?

isao commented 11 years ago

wild guess, maybe something to do with the stream finish ordering? joyent/node@c38ce9bc0a143141abfa638289b458ddaaac26d6

dominictarr commented 11 years ago

hmm, maybe. did you post an issue on node?

isao commented 11 years ago

Hi Dominic, sorry for delay. Filed issue with joyent/node.

isao commented 11 years ago

Ah, my repro case works fine in 0.11.3, 0.11.2. Problem is with my unit test code, I think.

Sorry I hadn't taken more time running this down-- through is pretty great. Thanks.

isao commented 11 years ago

Hi Dominic,

So actually the issue isn't with my test code (I still had the issue discussed above with never getting a "close" event, I just made a bad repro case).

But coincidentally I fixed a bug (isao/mojito-cli-create@2dea106b59f5da48ff118ff623cdd6837d4b3f2e) and all tests passed on node 0.11.2, 0.11.3. (The bug was my replacer code would "eat" a trailing newline, and just enqueue an empty string.)

My theory is this line in through requires buffer.length to be truthy to "end", but an empty string yielded a falsey buffer.length on node 0.11.2?

Node's buffer.js changes on June 18 do seems to include related changes.

To reproduce:

  1. clone https://gist.github.com/isao/5904358 && cd to it
  2. do npm i through
  3. do node index.js with node ~0.11.2 -- "done!" is not logged to the console.
  4. do node index.js with earlier versions of node, "done!" is logged to the console.
  5. apply the diff to replacer.js "done!" is logged to the console in all node versions.

Not sure if this is a documentation thing for through, or a small edge case, or what. Hope this is helpful at least.