Streams2 - base classes, zlib, crypto, fs #4348
Conversation
| options = options || {}; | ||
| this.fd = options.hasOwnProperty('fd') ? options.fd : null; | ||
| this.flags = options.hasOwnProperty('flags') ? options.flags : 'r'; | ||
| this.mode = options.hasOwnProperty('mode') ? options.mode : 438; /*=0666*/ |
There was a problem hiding this comment.
Wouldn't the in operator be more appropriate in such cases? I can imagine cases where you would want to have inheritance for settings. (This isn't a concern for this pr, but more for node in general.)
There was a problem hiding this comment.
I completely disagree. Keep in mind that you need to have setting stored in prototype object for this check to be false.
There was a problem hiding this comment.
var s = fs.createReadStream("/foo.txt", { __proto__: defaultOpts, etc: "etc" });And guess what happens when __proto__ either overwrites hasOwnProperty, or is null.
There was a problem hiding this comment.
Well, this one is certainly true, but lets face the truth. Users that are exploiting such stuff when using core APIs, are already doomed :)
There was a problem hiding this comment.
I actually like the ability to specify the default options at one place, and overwrite them as needed. In some cases, this can greatly reduce the amount of duplicated code. (Maybe I'm doomed, but at least I have readable programs ^^.)
Without looking at the source code, you'll never guess that it won't work.
There was a problem hiding this comment.
Well, lets do it this way then options.fd !== undefined ? ... : .... I really dislike in keyword.
There was a problem hiding this comment.
In this case, that's probably the better option, as it allows the cheap removal of settings (without switching the object to dictionary mode by using delete). Although you might want to use typeof options.fd != "undefined".
|
+1. I really like the new interfaces, although I'm afraid they'll affect speed in a serious manner. Are there any stats? |
| options = options || {}; | ||
|
|
||
| // cast to an int | ||
| this.bufferSize = ~~this.bufferSize; |
There was a problem hiding this comment.
Though, it's not really important.
|
Whoa, finally reviewed it... Looks good to me! :) |
|
And +1 @isaacs for such a big work! |
|
@fb55 I've been using the benchmark/net-pipe.js script against the streams2-net branch and master. There is a very very slight decrease in speed, but it's dwarfed by the overhead of IO. It'll be easier to really dig into that once it's working in http, because we have better tooling to analyze http performance, and that's what everyone cares about. Once the |
That is, the transform is triggered by a _read, not by a _write. This way, backpressure works properly.
It was testing the length *before* adding the current chunk, which is the opposite of correct. Also, the return value was flipped.
This fixes the CONNECT/Upgrade HTTP functionality, which was not getting sliced properly, because readable wasn't emitted on this tick. Conflicts: test/simple/test-http-connect.js
Otherwise resume() will cause data to be emitted before it can be handled.
Otherwise (especially with stdin) you sometimes end up in cases where the high water mark is zero, and the current buffer is at 0, and it doesn't need a readable event, so it never calls _read().
Necessary for proper stdin functioning
This is a combination of 6 commits.
* XXX net fixup lcase stream
* net: Refactor to use streams2
Use 'socket.resume()' in many tests to trigger old-mode behavior.
* net: Call destroy() if shutdown() is not provided
This is important for TTY wrap streams
* net: Call .end() in socket.destroySoon if necessary
This makes the http 1.0 keepAlive test pass, also.
* net wtf-ish stuff kinda busted
* net fixup
Unnecessary in streams2
Because of some of the peculiarities of http, this has a bit of special magic to handle cases where the IncomingMessage would wait forever in a paused state. In the server, if you do not begin consuming the request body by the time the response emits 'finish', then it will be flushed out. In the client, if you do not add a 'response' handler onto the request, then the response stream will be flushed out.
Streams2 style streams might have already kicked off a read() or write() before emitting 'data' events. Make the test less dependent on ordering of when data events occur.
This implements stream2 functionality for everything except net and http.
The tcp and pipe changes are working in the streams2-net branch, but they break http quite a lot, so it's taking some time to finish.
Please review, but I'd like to merge in as
--no-ffso that it's easier to revert if necessary.