Allow setting custom highWaterMark via node-fetch options (#386)

[xxczaki]

Fixes #386

This PR uses some bits from #563, but instead of passing the custom highWaterMark to clone method, it is passed via node-fetch options:

- fetch(url).then(res => res.clone(10).buffer());
+ fetch(url, {highWaterMark: 10}).then(res => res.clone().buffer());

I used the same tests from #563 to confirm that everything works as expected. However, it would be great if someone could review my changes to check whether I made any mistakes 😄

[bitinn]

Interesting, so you end up adding an extension to the clone() API.

I would say that's understandable, but wonder:

• Whether we should default to a larger highWaterMark on clone, current limit (16KB) is fairly low even for a JSON response, it seems fair to say people wouldn't want to use this API extension unless absolutely needed.

[xxczaki]

@bitinn Well, we can set a default highWaterMark value to match browser limits (about 1MB, not consistent in all browsers). Are there any downsides from using higher limit than default 16KB?

[bitinn]

@xxczaki some thoughts:

• In theory I don't want to set a larger highWaterMark at all, it basically means we are hiding an issue from users: that they didn't fully understand how stream works, and let 1 of the cloned stream buffer the entire content in memory instead. (because content flow into both cloned streams at the same time, if one is filling up, it will cause back-pressure on upstream, which pauses both streams.)

• But the reality is: browsers used to implement this freely, as Stream spec doesn't suggest a default value; Chrome used to buffer as much as possible until OOM, so users will never run into a back-pressure problem.

• Now that there is a CountQueuingStrategy, which means the number of chucks a stream can hold, I have been looking for a default value from major browsers, but haven't found one. And of course both highWaterMark and chunk size can be set based on platforms and devices...

So here is the thing:

• We introduce an API extension that will for sure make isomorphic users not happy.

• We can't share similar highWaterMark as browsers, and people will surely run into back-pressure issue not experienced in browsers, mostly because of their own misunderstanding.

• I don't know what we should do, let people experience their own fault or shield them from it in common cases.

• I would like to hear opinions.

[bitinn]

Is there a cleaner way to test these instead of timeout?

• let one of the clone stream flow,
• count bytes,
• let it back-pressure, make sure it's within expectation
• then observe the drain event on it
• letting the other stream flow.
• and know that it works as expected?

[bitinn]

ping @jimmywarting @gr2m for comments:

• Should we add this API extension?

• Should we set a larger highWaterMark on clone()?

• Otherwise, should we just provide a segment on how clone streams work in our readme?

See my comment above for why I don't like this solution: #671 (comment)

[gr2m]

I don't have strong opinions either way. If we introduce the new option then we should add documentation for it

[jimmywarting]

• In theory I don't want to set a larger highWaterMark at all, it basically means we are hiding an issue from users: that they didn't fully understand how stream works, and let 1 of the cloned stream buffer the entire content in memory instead. (because content flow into both cloned streams at the same time, if one is filling up, it will cause back-pressure on upstream, which pauses both streams.)

Fair point.

So here is the thing:

• We introduce an API extension that will for sure make isomorphic users not happy.
• We can't share similar highWaterMark as browsers, and people will surely run into back-pressure issue not experienced in browsers, mostly because of their own misunderstanding.
• I don't know what we should do, let people experience their own fault or shield them from it in common cases.

Sticking to our motto "stay as close to the spec as possible without much differences".

what's the status of bringing whatwg streams into node?
If we switch to whatwg streams would it break stuff?

could we maybe increase the/set highWaterMark by some % of available memory but maybe still limit it to something with Math.max?

I have not had any problem with how it currently works, never had to clone anything using node-fetch so i don't have a strong opinion either

[bitinn]

could we maybe increase the/set highWaterMark by some % of available memory but maybe still limit it to something with Math.max?

probably not safe, people do parallel fetching all the time, handling that means a lot more complexity.

what's the status of bringing whatwg streams into node?

Stream spec is getting more complex by the days, I just don't know if it's ever going to be in nodejs core. I do want to support the polyfill at some point though.

[xxczaki]

When creating this PR, I was following the suggested solution from this comment. I think what we should do, no matter what solution we choose, is to throw a specific error message, when the default highWaterMark limit is surpassed, so that users will know about the fact, that the default Node.js limit is different, than the one in the browser.

[bitinn]

When creating this PR, I was following the suggested solution from this comment.

It's indeed my suggestion to pass highWaterMark via options. Though I always have some reservations on fixing the problem this way. Let me explain below.

no matter what solution we choose, is to throw a specific error message, when the default highWaterMark limit is surpassed

I don't think this is possible or desirable: back-pressure is simply a core mechanism of streams, it keeps the memory usage of nodejs process to a reasonable level.

• We don't know the size of the stream beforehand, hence can't provide a message on clone() to inform users it may trigger back-pressure.
• And when back-pressure kicks it, it simply pause the upstream and wait for the blocking downstream to drain, there is no inherent issues here.
• What nodejs is saying: Users are solely responsible to let their downstream drain and not fill up.
• But to the user's eyes: Their isomorphic application appears to "hang forever", and "it doesn't happen in browsers".

So my reservation on the fix has been:

• The user will need to know their issue is caused by back-pressure to rise the highWaterMark cap.
• But if they already know that, they might as well realize the correct solution is to pipe both of your clone stream to somewhere.
• That is: say you have got a large JSON response stream, you want to run 2 different operations on it, so you clone it, then you are responsible to do them in parallel, not one after another.
• Because doing it in sequence will require the idle stream to hold all data in memory, causing back-pressure.

So the solution we offer here is for users to be lazy, "just hold all of them in memory instead", well, my question is: if you don't care about memory, why use stream or clone? why not get a buffer() or text() or blob()?

/rant over

[bitinn]

But all in all, I think users aren't going to think much on this:

• That clone is essentially tee-ing the response stream (as defined in Fetch spec);
• But instead copy-ing the content of response object (this is much more nature);
• Fetch is after all, an easy to use, low-level API.

So I say, fine, merge this PR, give users an easy way out ^_^

[bitinn]

(by the way, our tests actually illustrate 2 ways a cloned response can be used, and the reason people keep running into this problem is because they chose the sequential version, it's more nature to think that way...)

[xxczaki]

Before merging this PR, I will document this option ;)

[bitinn]

I think we still need a doc section specifically to explain “why” and “why not” to change the highWaterMark, and some examples regarding how to resolve clone() stream in parallel, anyone up for the PR? (From my phone)

…

On Sep 25, 2019, at 3:00, Antoni Kepinski ***@***.***> wrote: Merged #671 into 3.x. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[xxczaki]

@bitinn I agree, we should also edit the LIMITS.md file and update the section related to highWaterMark.

[keesvanlierop]

If I could briefly comment on this, I've noticed that in the scenario that a cloned response gets cloned, highWaterMark also needs to be passed in the response options to prevent it from being undefined.