streams: update .readable/.writable to false #1217
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/cc @chrisdickinson @iojs/streams |
|
I like it. |
|
+1 |
| @@ -660,7 +661,7 @@ Readable.prototype.on = function(ev, fn) { | |||
| this.resume(); | |||
| } | |||
|
|
|||
| if (ev === 'readable' && this.readable) { | |||
| if (ev === 'readable') { | |||
There was a problem hiding this comment.
Curious: why did this have to change?
There was a problem hiding this comment.
Now that .readable can possibly be false, the whole conditional has a chance of evaluating to false, which causes test failures. Since the second condition was always evaluating to true before this, it can be safely removed and all tests pass again after this particular change.
There was a problem hiding this comment.
Since the second condition was always evaluating to true
That's not necessarily the case, different subclasses may be manipulating that attribute. This may end up changing the behavior of process.stdout.on('readable'), or adding a readable listener to a destroyed TLSSocket.
There was a problem hiding this comment.
If that can be a problem, then we need to add tests for that or tweak that relevant code possibly.
There was a problem hiding this comment.
Would you mind adding those tests as a part of this PR?
|
cc'ing @isaacs to see if he can give us some background on why the |
|
Cool, looks like we've got the feedback we need to go ahead with this. I'd like to make sure we're not going to break anybody's code with this. The |
Reviewed-By: James M Snell <[email protected]> PR-URL: nodejs#3463
If a tab completion is attempted on an undefined reference inside of a function, the REPL was exiting without reporting an error or anything else. This change results in the REPL reporting the ReferenceError and continuing. Fixes: nodejs#3346 PR-URL: nodejs#3358 Reviewed-By: James M Snell <[email protected]>
v8 is faster at setting object properties in JS than C++. Even when it
requires calling into JS from native code. Make
process._getActiveRequests() faster by doing this when populating the
array containing request objects.
Simple benchmark:
for (let i = 0; i < 22; i++)
fs.open(__filename, 'r', function() { });
let t = process.hrtime();
for (let i = 0; i < 1e6; i++)
process._getActiveRequests();
t = process.hrtime(t);
console.log((t[0] * 1e9 + t[1]) / 1e6);
Results between the two:
Previous: 4406 ns/op
Patched: 690 ns/op 5.4x faster
PR-URL: nodejs#3375
Reviewed-By: James Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Updated test-npm to use absolute paths for tmp/cache/prefix PR-URL: nodejs#3309 Reviewed-By: Rod Vagg <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>
Emitting 'close' before the history has flushed is somewhat incorrect and rather confusing. This also makes the 'close' event always asynchronous for consistency. Refs: nodejs#2356 PR-URL: nodejs#3435 Reviewed By: Evan Lucas <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
Refs: nodejs#3308 PR-URL: nodejs#3489 Reviewed-By: Rod Vagg <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>
Refs: nodejs#3308 PR-URL: nodejs#3489 Reviewed-By: Rod Vagg <[email protected]>
PR-URL: nodejs#3310 Reviewed-By: Jeremiah Senkpiel <[email protected]>
Pull in the change that has been committed to v8 master in https://codereview.chromium.org/1415463002/. We are currently cherry-picking into 4.6 and 4.7 but until next next v8 update into Node Master I'd like to float it as it will make PPC LE go green in the CI Fixes: nodejs#3390 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> PR-URL: nodejs#3474
* Add a simple example for buffer.concat * Change grammar slightly. Fixes: nodejs#3219 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trevor Norris <[email protected]> PR-URL: nodejs#3255
41923c0 broke things. This fixes them. Reviewed-By: James M Snell <[email protected]> Reviewed-By: Evan Lucas <[email protected]> PR-URL: nodejs#3494
This update fixes a breaking regression in the date parser. Ref: https://code.google.com/p/chromium/issues/detail?id=539813 Ref: https://code.google.com/p/chromium/issues/detail?id=543320 PR-URL: nodejs#3484 Reviewed-By: Ben Noordhuis <[email protected]>
This PR is the first step enabling support for native modules
for AIX. The main issue is that unlike linux where all
symbols within the Node executable are available to the shared
library for a native module (npm), on AIX the symbols must
be explicitly exported. In addition, when the shared library is
built it must be linked using a list of the available symbols.
This patch covers the changes need to:
1) Export the symbols when building the node executable
2) Generate the file listing the symbols that can be used when
building the shared library.
For AIX, it breaks the build process into 2 steps. The first builds
a static library and then generates a node.exp file which contains
the symbols from that library. The second builds the node executable
and uses the node.exp file to specify which symbols should be
exported. In addition, it save the node.exp file so that it can
later be used in the creation of the shared library when building
a native module.
The following additional steps will be required in dependent projects
to fully enable AIX for native modules and are being worked
separately:
- Updates to node-gyp to use node.exp when creating the
shared library for a native module
- Fixes to gyp related to copying files as covered in
https://codereview.chromium.org/1368133002/patch/1/10001
- Pulling in updated gyp versions to Node and node-gyp
- Pulling latest libuv
These changes were done to minimize the change to other platforms
by working within the existing structure to add the 2 step process
for AIX without changing the process for other platforms.
PR-URL: nodejs#3114
Reviewed-By: Ben Noordhuis <[email protected]>
This test is already partially disabled for several platforms with the comment that the required info is not provided at the C++ level. I'm adding AIX as and PPC BE linux as they currently fall into the same category. We are working to see if we can change that in v8 but it will be non-trivial if is possible at all so I don't want to leave the CI with failing tests until that point. PR-URL: nodejs#3491 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>
PR-URL: nodejs#3478 Reviewed-By: Trevor Norris <[email protected]>
malloc(0) and realloc(ptr, 0) have implementation-defined behavior in that the standard allows them to either return a unique pointer or a nullptr for zero-sized allocation requests. Normalize by always using a nullptr. Fixes: nodejs#3496 PR-URL: nodejs#3499 Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
In fs.readFile, if an encoding is specified and toString fails, do not throw an error. Instead, pass the error to the callback. Fixes: nodejs#2767 PR-URL: nodejs#3485 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
Currently there are many instances where assert.fail is directly passed to a callback for error handling. Unfortunately this will swallow the error as it is the third argument of assert.fail that sets the message not the first. This commit adds a new function to test/common.js that simply wraps assert.fail and calls it with the provided message. Tip of the hat to @Trott for pointing me in the direction of this. PR-URL: nodejs#3453 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Rich Trott <[email protected]>
The test sometimes fail on an assertion but no useful error message was generated for debugging. Modify the test to generate useful debugging message. PR-URL: nodejs#3501 Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Rich Trott <[email protected]>
If the resulting buffer.toString() call in fs.read throws, catch the error and pass it back in the callback. This issue only presents itself when fs.read is called using the legacy string interface: fs.read(fd, length, position, encoding, callback) PR-URL: nodejs#3503 Reviewed-By: Trevor Norris <[email protected]>
inherits is used in lib and tests but its functionality itself is not tested yet. PR-URL: nodejs#3507 Reviewed-By: Sakthipriyan Vairamani <[email protected]>
This was referenced Jul 8, 2023
This was referenced Nov 30, 2023
This was referenced Nov 30, 2023
This was referenced Dec 1, 2023
This was referenced Dec 2, 2023
This was referenced Dec 2, 2023
This was referenced Dec 3, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These properties were initially used to determine stream status back in node v0.8 and earlier. Since streams2 however, these properties were always true, which can be misleading for example if you are trying to immediately determine whether a Writable stream is still writable or not (to avoid a "write after end" exception).
This commit avoids having to dig into internal stream state to know whether you can still read/
push()with a Readable stream and whether you can stillwrite()/end()a Writable stream at any given time. It also makes the behavior of.readable/.writableconsistent with other core modules that use properties with the same names (e.g.net.Socket).