search

mafintosh / tar-stream

tar-stream is a streaming tar parser and generator.
MIT License
411 stars 93 forks source link

Fix #123 - compatibility with native node@14 streams #124

Closed tex0l closed 4 years ago

tex0l commented 4 years ago

The reason why tar-stream breaks with node@14 native streams is because of this feature introduced in node v14.0.0: https://github.com/saitonakamura/node/commit/b004ac0edf815d6bd3dd9cdaa8c24be3f72cff82

Basically, when calling the end method on a Writable stream, it triggers its destruction by default, which was not the case in earlier versions.

In tar-stream, the reason why it causes an issue is that there are as many Source streams as there are entries. Each of those are PassThrough streams with an offset on the Extract stream. When the first entry ends, it consequently triggers the destruction of this Source stream, which is manually forwarded to the parent stream:

https://github.com/mafintosh/tar-stream/blob/81cd7390f091244a51e0ea67be3c454352f8b366/extract.js#L37-L39

which destroys the Extract stream without triggering neither an error nor the finished event.

By setting an autoDestroy: false when instantiating the Source stream it forces the same behaviour as in previous versions of node, and does not break anything.

I've updated the travis-ci configuration to test against node up to v14, and to test with native implementations of streams starting from node 8 (I've tried using the native streams on node@6, it breaks).

arantes555 commented 4 years ago

Impressive debugging. I had completely missed that this BC in node could affect this behaviour. Well done.

mafintosh commented 4 years ago

Thanks! I disabled the travis check cause that's not part of our supported API, but happy to add this as it helps with rollup

mafintosh commented 4 years ago

2.1.3

tex0l commented 4 years ago

Fair enough, thank you!

developit commented 4 years ago

Oh my goodness I am so glad someone PR'd this change! I spent ages a couple weeks ago coming to the same fix, should have thought to PR it at the time. Way to go @tex0l!