search

sindresorhus / get-stream

Get a stream as a string, Buffer, ArrayBuffer or array
MIT License
341 stars 33 forks source link

v7.0.0 change in behavior with large sizes #52

Closed XhmikosR closed 1 year ago

XhmikosR commented 1 year ago

I'm trying to update to v7.0.0 and I'm getting a weird behavior. Same code, just the get-stream update to v7.0.0: https://github.com/XhmikosR/download/compare/master...XhmikosR:download:update-get-stream

Promise returned by test never resolved

Tests work up to 16_383 bytes. Anything larger than that is failing. Going back to get-stream v6.0.1 works fine as one can see by CI.

@sindresorhus any ideas why this happens?

Happens on Linux/Windows on Node.js 16, 18 and 20 (CI is available to check too).

Thanks!

EDIT: Looking at the Node.js docs, I found highWaterMark that's 16384 bytes: https://nodejs.org/docs/latest-v16.x/api/stream.html#new-streamreadableoptions. Could this be related?

XhmikosR commented 1 year ago

So, this seems to work from a quick test for my case:

-   const stream = new PassThroughStream();
+   const stream = new PassThroughStream({highWaterMark: 8_000_000});

The problem is one cannot pass Number.POSITIVE_INFINITY as a highWaterMark value, and I'm not sure if it would be safe anyway.

There must be another option here to do it properly, though.

malte94 commented 1 year ago

I have possibly the same issue with getStreamAsBuffer. I assume a "highWaterMark" cannot be passed there?

XhmikosR commented 1 year ago

I'm not super familiar with streams, but even if one could pass a higher highWaterMark value, I don't think it's the right approach.

The right approach would be to listen on an event until there are no more data I suppose.

sindresorhus commented 1 year ago

The issue is most likely a bug in Node.js with pipeline (it's known to be buggy).

sindresorhus commented 1 year ago

I will do the highWaterMark fix for now (https://github.com/sindresorhus/get-stream/releases/tag/v7.0.1), but it would be helpful if anyone would submit a failing test for this, so I can look into a proper fix.

malte94 commented 1 year ago

@sindresorhus Thank you very much for your quick reply and taking care of this! 🙏 For me, your fix seems to be working fine.

ehmicky commented 1 year ago

This issue should now be fixed by 8.0.0. :tada: Should we close it?