Cleanup stream state in net

[chrisdickinson]

part of #445

This set of patches removes much of the private state manipulation net.Socket was doing:

• 58912a0: Removes the this._writableState.decodeStrings = false in favor of always setting options.decodeStrings = false before stream.Duplex.call(this, options).
• ee369cd: Instead of manually setting this._readableState.flowing = false, call this.pause(). Since we have not returned from object instantiation, there should be no side effects.
• 9c26dd1: A bit trickier: only destroy in onSocketFinish we are already not readable – whether that's because we started as readable = falseor because we're in the dead zone between ended = true and readable = false. What this means in practice is that sockets that would have been destroyed here will instead be destroyed by onSocketEnd (as that nextTick'd function completes.)
• 29e7d80: Maybe controversial, but: the errorEmitted attribute was only made available for the purposes of net.Socket (and tls.TLSSocket), and even then only for errors that they can intercept themselves. I moved that attribute down to those child classes, because there's no need within Writable to track that information (basically trying to localize the illicit information spillage).

R=@rvagg, @isaacs, or @bnoordhuis ?

[piscisaureus]

Can you assess the semver impact of these patches? Do you think it's all okay to do in a patch release (no api changes) ?

[chrisdickinson]

@piscisaureus I'm going to pull some npm packages today and run tests. It passes make test-ci, but I'd like to make doubly sure.

[Qard]

Is this blocked on anything?

[chrisdickinson]

Not at present. I'd like to bring it in for the minor release this week if possible.

[indutny]

Hm... This was a bug fix, as far as I remember. Why doesn't it apply anymore?

[indutny]

Some nits, otherwise LGTM. Though, I'd like to have one more LGTM from the @piscisaureus or @bnoordhuis or @isaacs

[Fishrock123]

Can the CI be run on this after fixups?

Edit: looked at commits, may not be necessary?

[rvagg]

@Fishrock123 I wouldn't mind giving you access to CI so you can be more proactive since you're one of the most attentive to the repo. Let me know if you're interested and I'll send you details via email.

[cjihrig]

@rvagg I would like access to the CI please.

[rvagg]

@cjihrig of course! will get you set up and email.

[Fishrock123]

@rvagg sure. (just let me know what I should look for to run it on, I guess haha)

[indutny]

Hm... doesn't look like it belong here.

[brendanashworth]

Looks like this pull request could use a rebase before it is merged @chrisdickinson. Besides that, is there anything holding this back from being merged?

[Fishrock123]

ping @chrisdickinson

[Qard]

FYI: Seems to have one trivial merge conflict due to a change of this.ssl to just ssl in b968623#diff-04c3a6bcf355f2e05e7700c1b253d475L310.

Also, one test is failing for me:

/usr/bin/python tools/test.py --mode=release message parallel sequential -J
=== release test-tls-econnreset === Path: parallel/test-tls-econnreset
assert.js:88
throw new assert.AssertionError({
^
AssertionError: false == true
at TLSSocket.<anonymous> (/home/sbelanger/Documents/io.js/test/parallel/test-tls-econnreset.js:60:5)
at emitOne (events.js:77:13)
at TLSSocket.emit (events.js:166:7)
at TLSSocket._tlsError (_tls_wrap.js:467:10)
at TLSWrap.ssl.onerror (_tls_wrap.js:355:12)
Command: out/Release/iojs /home/sbelanger/Documents/io.js/test/parallel/test-tls-econnreset.js
[00:42|% 100|+ 840|- 1]: Done make: *** [test] Error 1

I haven't looked any closer than that.

[jbergstroem]

@chrisdickinson just following up here. Could you rebase so we can revisit?

[chrisdickinson]

Closing this because it feels a bit dicey. If someone else is interested in taking up the cause, feel free!