src: nix stdin _readableState.reading manipulation #454
Conversation
|
LGTM |
|
@chrisdickinson Maybe R= someone? (Not me, I've taken a solemn vow to never touch anything involving streams.) |
|
It does set The only thing I was wondering is that it looks like before it sets that property it would create a 0 length buffer (#L98-L104) and because it's paused in both cases it would add it to the But this would then get handled in the relevant way by But once the interactions are changed from manually manipulating state to triggering API, the underlying implementation detail could be changed in the readable-stream to make this action do less work. I guess it depends if we want calling |
It's easier to think of
Yep! In particular we could look into the work necessary to not push zero-length buffers onto the queue. 👋 |
|
@chrisdickinson Yeah, that does make sense for streams of text and buffers, would we do the same in #L139 seems to signify that we have just pushed some data and that data is being processed to be emitted if in flowing mode. In every event, a stream could even be in a state where it is actually awaiting data, but is not paused or actively reading as well potentially. What I stated before:
was actually wrong anyway #L160 is where state.objectMode || chunk && chunk.length > 0would be false (#127) because of the zero length. (That will teach me to be reading code at 2am!:weary:) @bnoordhuis IMO I think this PR is safe to merge (assuming all tests pass - I have not checked out the branch) although it would be good to get a extra pair of eyes on it (the more the better I always say) even if just for educational purposes. I can see why you might have vowed to avoid touching streams 😄 I guess this is why we need to make them simpler, more friendly and approachable. |
|
-1. While doing
|
|
@vkurchatkin I agree that it is not very intuitive, however the sooner we can remove implementation detail from code which is using it, we can develop the implementation and improve the API which is utilised. Also |
|
Quote from docs:
|
|
That file is implementing the stream, not consuming it. It's a lot easier to reason about streams when the only way of affecting their private state is through their public API – which
|
| @@ -561,7 +561,7 @@ | |||
| // not-reading state. | |||
| if (stdin._handle && stdin._handle.readStop) { | |||
| stdin._handle.reading = false; | |||
| stdin._readableState.reading = false; | |||
| stdin.push(''); | |||
There was a problem hiding this comment.
Just curious. Will this create an empty buffer every time it does this?
|
One comment, otherwise LGTM if tests are passing. |
|
@indutny I believe it will if the Readable.prototype.push = function(chunk, encoding) {
var state = this._readableState;
if (!state.objectMode && typeof chunk === 'string') {
encoding = encoding || state.defaultEncoding;
if (encoding !== state.encoding) {
chunk = new Buffer(chunk, encoding);
encoding = '';
}
}
return readableAddChunk(this, state, chunk, encoding, false);
};v1.x/lib/_stream_readable.js#L95-L107 However once it enters into |
|
There is an interesting part in the documentation I stumbled across, which states:
https://github.com/iojs/io.js/blob/v1.x/doc/api/stream.markdown#streampush 😈 |
|
@sonewman Hah, yeah. I saw that too – though I imagine it's still better to use a documented (if harshly worded) part of the public API than to reach in and manipulate private state. |
|
@chrisdickinson I think this could be considered a valid use case, since the side effects are what we are trying to attain. It puts the stream in a It would be nice if we didn't have to make the zero length if (!state.objectMode && chunk && typeof chunk === 'string') |
In certain environments escape sequences could be splitted into multiple chunks. For example, when user presses left arrow, `\x1b[D` sequence could appear as two keypresses (`\x1b` + `[D`). PR-URL: #1601 Fixes: #1403 Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Roman Reiss <[email protected]>
dgram#send callback was changed synchronously. The PR-URL is here joyent/libuv#1358 This commit is temporary fix until libuv issue is resolved. libuv/libuv#301 PR-URL: #1313 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
AES-GCM or CHACHA20_POLY1305 ciphers must be used in current version of Chrome to avoid an 'obsolete cryptography' warning. Prefer 128 bit AES over 192 and 256 bit AES considering attacks that specifically affect the larger key sizes but do not affect AES 128. PR-URL: #1660 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Shigeki Ohtsu <[email protected]> Reviewed-By: Roman Reiss <[email protected]>
Certain cases with comments inside arrays or object literals fail to pass eslint's comma-spacing rule. This change sets the comma-spacing rule to the 'warn' level. Once eslint/eslint#2408 is resolved and released, this rule should be set back to 'error' level. PR-URL: #1672 Reviewed-By: Yosuke Furukawa <[email protected]>
Snapshots had been previously disabled because of a security vunerability. This has been fixed (ref: #1631 (comment)) Also, re-enable snapshots for ARMv6 builds. There were previous build issues that have been fixed. Fixes: #1631 PR-URL: #1663 Reviewed-By: Ben Noordhuis <[email protected]>
Fixes: #1676 PR-URL: #1678 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
Provide more information in `ares_txt_reply` to coalesce chunks from the same record into one string. fix #7367
On Windows, when compiling with `UNICODE` defined, `LoadLibrary` becomes `LoadLibraryW`. When an ASCII string is passed to that function it crashes. PR-URL: #226 Reviewed-By: Bert Belder <[email protected]>
Previously, in the event of an unhandled error event, if the error is a not an actual Error, then a default error is thrown. Now, the argument is appended to the error message and added as the `context` property of the error. PR-URL: #1654 Reviewed-By: Rod Vagg <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Extracts test-npm from Makefile and puts it in tools/test-npm.sh Also improves test-npm to use a separate copy of deps/npm for testing. PR-URL: #1662 Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: Evan Lucas <[email protected]>
The old pattern didn't include files in lib/internal. This changes the pattern to directories which makes eslint apply to all subdirectories as well. PR-URL: #1686 Reviewed-By: Chris Dickinson <[email protected]>
PR-URL: #1680 Reviewed-By: Rod Vagg <[email protected]>
The readfile/pipe tests rely on pre-existing pipes in the system. This arguably tests the OS functionality and not really io.js functionality. Removing TODOs. PR-URL: #2033 Reviewed-By: Trevor Norris <[email protected]>
closes: #41 PR-URL: #2037 Reviewed-By: Ben Noordhuis <[email protected]>
PR-URL: #2034 Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
PR-URL: #2048 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>
PR-URL: #850 Reviewed-By: Stephen Belanger <[email protected]>
vcbuild.bat calls python configure before setting GYP_MSVS_VERSION, so SelectVisualStudioVersion (tools\gyp\pylib\gyp\MSVSVersion.py) defaults to 'auto' and selects VS 2005. vcbuild sets the environment in the current shell, so this issue would manifest itself only on the first invocation of the script in any given shell windows. Reviewed-By: Julien Gilli <[email protected]> PR-URL: nodejs/node-v0.x-archive#20109
PR-URL: #2036 Reviewed-By: Alexis Campailla <[email protected]>
PR-URL: #2036 Reviewed-By: Alexis Campailla <[email protected]>
Instead of not running the dgram-bind-shared-ports on Windows, check that it gets ENOTSUP. PR-URL: #2035 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: #1938 Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Alexis Campailla <[email protected]>
On upgrading openssl, all symlinks in pulic header files are replaced with nested include files. The issue was raised that installing them leads to lost its references to real header files. To avoid this, all public header files are copied into the `deps/openssl/openssl/include/openssl/` directory. As a result, we have duplicated header files under `deps/openssl/openssl/` but copied files are refereed in build as specified to include path in openssl.gyp. Fixes: #1975 PR-URL: #2016 Reviewed-By: Rod Vagg <[email protected]> Reviewed-By: Johan Bergström <[email protected]>
to replace the full src download by node-gyp, using the proper format instead of the full source format PR-URL: #1975 Reviewed-By: William Blankenship <[email protected]> Reviewed-By: Johan Bergström <[email protected]>
Refs: #2050 PR-URL: #2053 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Use vm.isContext() to properly identify contexts. PR-URL: nodejs/node-v0.x-archive#25382 PR-URL: #2052 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
Fix the regexp used to detect 'Unexpected token' errors so that they can be considered as recoverable. This fixes the following use case: > var foo = 'bar \ ... baz'; undefined > foo 'bar baz' > Fixes: nodejs/node-v0.x-archive#8874 PR-URL: nodejs/node-v0.x-archive#8875 PR-URL: #2052 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
test-repl-tab-complete.js contains numerous assertions that are never run. Anything that results in a ReferenceError bails out, and never calls the functions containing the assertions. This commit adds checking for successful tab completions, as well as ReferenceErrors. PR-URL: #2052 Reviewed-By: Ben Noordhuis <[email protected]>
Break up Buffer#toString() into a fast and slow path. The fast path optimizes for zero-length buffers and no-arg method invocation. The speedup for zero-length buffers is a satisfying 700%. The no-arg toString() operation gets faster by about 13% for a one-byte buffer. This change exploits the fact that most Buffer#toString() calls are plain no-arg method calls. Rewriting the method to take no arguments means a call doesn't go through an ArgumentsAdaptorTrampoline stack frame in the common case. PR-URL: #2027 Reviewed-By: Brian White <[email protected]> Reviewed-By: Christian Tellnes <[email protected]> Reviewed-By: Daniel Cousens <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
PR-URL: #2042 Reviewed-By: Brendan Ashworth <[email protected]> Reviewed-By: Johan Bergström <[email protected]>
Tests in the disabled directory are not used by Makefile nor by the CI. Other than a single 2015 commit that puts 'use strict' in each test, many of them haven't been touched in years. This removes all the disabled tests that have been unmodified since 2011 (with the exception of the 'use strict' modification mentioned above). PR-URL: #2045 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Evan Lucas <[email protected]>
If an object's prototype is munged it's possible to bypass the instanceof check and cause the application to abort. Instead now use HasInstance() to verify that the object is a Buffer, and throw if not. This check will not work for JS only methods. So while the application won't abort, it also won't throw. In order to properly throw in all cases with toString() the JS optimization of checking that length is zero has been removed. In its place the native methods will now return early if a zero length string is detected. Ref: #1486 Ref: #1922 Fixes: #1485 PR-URL: #2012 Reviewed-By: Ben Noordhuis <[email protected]>
Prevent debug call from showing [object Object] for dnsopts in lookupAndConnect PR-URL: #2059 Reviewed-by: Colin Ihrig <[email protected]>
this opts for stream.push('') which has the same effect
but uses a public API.
this opts for stream.push('') which has the same effect
but uses a public API.
PR-URL: #454
Reviewed-By: Fedor Indutny <[email protected]>
|
Merged (finally!) in 8cee8f5. Thanks @Qard, @brendanashworth, for pinging me about this. Mea culpa on letting it sit so long :| |
this opts for stream.push('') which has the same effect
but uses a public API.
PR-URL: nodejs#454
Reviewed-By: Fedor Indutny <[email protected]>
this opts for
stream.push('')which has the same effect but uses a public API.(see #445)
R=@indutny