http_client: ensure empty socket on error

[indutny]

Read all pending data out of the socket on error event and ensure that
no data/end handlers will be invoked on socket.destroy().
Otherwise following assertion happens:

AssertionError: null == true
    at TLSSocket.socketOnData (_http_client.js:308:3)
    at TLSSocket.emit (events.js:107:17)
    at TLSSocket.Readable.read (_stream_readable.js:373:10)
    at TLSSocket.socketCloseListener (_http_client.js:229:10)
    at TLSSocket.emit (events.js:129:20)
    at TCP.close (net.js:476:12)

Fix: nodejs/node-v0.x-archive#9348
See: npm/npm#7349

cc @iojs/collaborators @bnoordhuis @othiym23

[rvagg]

I'll hold up 1.5.1 for this

[rvagg]

oh, that was a lgtm from me

[rvagg]

https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/264/

[indutny]

@rvagg thanks! This seems to be pretty sensitive piece of code, would anyone from @iojs/collaborators like to take a look at it too and give it additional LGTY?

[indutny]

@rvagg erm, language is my enemy :) I didn't meant to offend you in any way, just would like to have two LGTYs on this.

[rvagg]

heh, no offense taken at all! I'm keen for more eyes too, mine aren't tuned to this section of code

[micnic]

LGTM! the only thing I would like to mention is to leave the declaration of the parser variable at the line 250.

[indutny]

@micnic : why? It might disappear after socket.read()

[indutny]

Landed in 1a3ca82, thank you!

[indutny]

@rvagg : guess we are good to go for 1.5.1 now. There is a remaining TLS leak that I'll investigate soon, but do not let it block the release for a significant time.

[indutny]

@rvagg : I mean - we already fixed most of it, only minor problem is left.

[micnic]

@indutny, I mentioned that for the sake of "variable declaration first in function body" code style. Could you show me, please, where exactly may the parser change in socket.read()? I didn't know about this, thought it is something more like a trivial stream.read() implementation.

[indutny]

@micnic this is not about stream.read() itself, it might invoke socketOnData or socketOnEnd which might call freeParser. In such case socket.parser will be null after stream.read() and the code below it will double-free the parser.

[micnic]

Thanks, I was a bit confused, I'll investigate more to understand better how the things work under the hood.

[rvagg]

is this an 0.12 & io.js thing only or does it also go to 0.10?

[indutny]

@rvagg 0.10 doesn't read from the socket on close event. This change ( f153d6d ) wasn't backported there.