Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Stream readable last 0.10 fixes #4934

Closed
wants to merge 8 commits into from

Conversation

isaacs
Copy link

@isaacs isaacs commented Mar 6, 2013

A few last fixes for Readable streams to get in before 0.10.

Sits on top of #4930. Just got tired of sending a separate PR for each commit.

@indutny
Copy link
Member

indutny commented Mar 6, 2013

A few?

Extremely handy when tracking down a flood of recursive nextTick warnings.
If the consumer of a Readable is asking for N bytes, and N > hwm,
then clearly we have set the hwm to low, and ought to increase it.

Fix nodejs#4931
It's not actually necessary for backwards compatibility, isn't
used anywhere, and isn't even tested.  Better to just remove it.
Now that highWaterMark increases when there are large reads, this
greatly reduces the number of calls necessary to _read(size), assuming
that _read actually respects the size argument.
@@ -162,6 +161,18 @@ Readable.prototype.setEncoding = function(enc) {
this._readableState.decoder = new StringDecoder(enc);
};

var MAX_HWM = Math.pow(2, 30) - 1;
function raiseHWM(n) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should it be called roundUp?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or powerOfTwo

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's only raising it if it's smaller than the max. But sure, roundUpToNextPowerOf2 is fine.

This prevents excessively raising the buffer level in tiny increments in
pathological cases.
@indutny
Copy link
Member

indutny commented Mar 6, 2013

LGTM!

@isaacs
Copy link
Author

isaacs commented Mar 6, 2013

Landed on master. Thanks!

@isaacs isaacs closed this Mar 6, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants