https2 breaks compatibility with https1

[mmalecki]

This bug was originally spotted in npm, issue isaacs/npm#1198.

When using new https2, callback from line https://github.com/isaacs/npm/blob/master/lib/utils/fetch.js#L87 doesn't get executed, because of set agent parameter.

https2 checks only for undefined agent value (https://github.com/joyent/node/blob/master/lib/https2.js#L69 ), while https1 checked for false (https://github.com/joyent/node/blob/master/lib/https.js#L96 ) as well.
(Spotted by @thejh)

Test case is here: https://gist.github.com/1145067
When node is running with --use-http1, everything works fine.

Also, I was told to @mikeal as he wrote https2.

[thejh]

The failing test case does create a connection to the registry, but it doesn't use TLS, it just sends a plain http request, which the server doesn't respond to, therefore nothing happens. My suggestion is to set a useTLS flag or so in https2:request if agent===false and then, in http2.js, to use something like this on line 1041:

self.onSocket((useHttps?tls.connect:net.createConnection)(options.port, options.host))

[koichik]

another way is here.
https://gist.github.com/1145963

[mikeal]

i'll get to this today, it's not a very difficult fix.

[mikeal]

please review @bnoordhuis or @isaacs or @ry

[ry]

@mmalecki - we would like to take your code but we require a contributor agreement, which can be signed electronically at http://nodejs.org/cla.html , before we can pull it into the repository.

Be sure to integrate the test and assert that @mikeal's fix does indeed fix it.

[mikeal]

@ry just wait a minute, i have a better fix. it's all my code so don't worry abou @mmalecki getting a CLA in

@mmalecki you should sign the CLA anyway for next time :)

[mikeal]

ok, much better fix now.

in all conditions we get the options object to tls and http.js doesn't require() tls, ever.

using net.connect() didn't work for me because, i think it's only there with a non-default configure option.

also, @bnoordhuis yes, one line is 84 chars long, it looks way uglier if i break it up, there is a long line already in the file 20 lines up :)

[mmalecki]

@mikeal, @ry
I've signed it few hours ago, so feel free to use my code.
I'm writing the test right now, I will do a pull request when I'm done (few moments).

[mikeal]

the test looks good. +1

[ry]

sorry that was confusing because i landed it originally in a different branch.

test 94963ab
fix 103990b

thanks everyone