docs: missing prefinish writable stream event

[mscdex]

This bit me tonight. I had a script that worked fine in node 0.10.33 but 0.11.14 was calling finish on next tick or later, causing unexpected behavior. After diving into the code I found that there is now also a prefinish event that I now listen on for node 0.11+.

It seems like if backwards compatibility can't be kept here, that prefinish should at least be publicly documented along with information comparing it to finish and how it all deviates from node 0.10 behavior.

[misterdjules]

Are you able to share this code with us, or at least the relevant part?

[mscdex]

Sure, it's here.

Without the prefinish handler, I get stream.push() after EOF on just about every node v0.11.x release.

To reproduce, comment out the .on('prefinish', onFinish'); line and then run node test/test-client-server.js with both node v0.10.33 and v0.11.14. It works just fine on v0.10, but crashes with that error on v0.11. Uncomment the line and everything works again on both node versions.

I don't have time at the moment to create a minimal test case for this, but it seems the change was introduced in c38ce9bc0a.

[misterdjules]

I couldn't run node test/test-client-server.js. It seems that this test is only present in ssh2's branch named "rewrite". However, running npm install in this branch results in an error because it can't find the npm module ssh2-streams. What can I do to run this test?

Nonetheless, I investigated why the finish event could not be emitted in some cases, and I came up with a change and a test. Could you please let me know if that's related to your issue?

Thank you!

[mscdex]

It's not that it isn't emitting finish, but it's emitting it later than it does in node v0.10. It seems that prefinish has taken the place (time-wise) of finish from node v0.10 and now finish in v0.11 has some new meaning?

Sorry about the lack of instructions. You should just need to checkout the rewrite branch and then do npm install https://github.com/mscdex/ssh2-streams/tarball/master in the rewrite's root directory.

[misterdjules]

Sorry, I had read your second comment only and lost the context. I'll take another look later, thank you for the instructions on how to install ssh2-streams.

[jasnell]

@mscdex ... still something you'd like to see? have time to do a PR?

[mscdex]

@jasnell Yes, it is still something I'd like to see. However I don't know the reasons for the change in behavior and all that, so I'm probably not the best one to write documentation for it.

[jasnell]

@chrisdickinson @trevnorris ... are either of you able to clarify the change that was made here?