Remove tls.createSecurePair usage, closes #515

[joux3]

The changes replace tls.createSecurePair with TLSSocket & DuplexPair on Node 8 and newer.

Some notes:

• Node 6 & 7 crashes on TLSSocket when a DuplexPair socket passed to the constructor. Therefore the old tls.createSecurePair is still used for anything before Node 8.
• the Sinon fake timers used in connection-retry-test.js somehow break DuplexPair internally, the bytes just aren't passed through it. Removing the timer faking does not cause the tests to fail when using an encrypted connection, but it does cause a failure when using an unencrypted connection (so probably just based on luck regarding timing). Any ideas how to address this? Use the fake timers if the connection used is unencrypted? Or maybe debug the root cause further? Faking just setTimeout seems to fix DuplexPair.

[arthurschreiber]

Heya 👋

Thanks for preparing this pull request - this is looking great! I looked into the crash on Node.js <= 7, and this is caused by some weird interaction between readable-stream and Node.js internals. If I change both tedious and duplexpair to use stream instead of readable-stream, no crash happens and the same code can be shared between all node versions.

When looking at the TLSSocket code between node v4-v7, and node v8+, it's easy to spot the cause of the crash:

This here is the code for earlier node versions: https://github.com/nodejs/node/blob/99af46e92ccde6a42dbaec1d51bca8b6d8e43236/lib/_tls_wrap.js#L260-L266

You can see that the code explicitly checks whether the given socket is an instance of Duplex (require('stream').Duplex) before it wraps it into a StreamWrap.

Unfortunately, require('readable-stream').Duplex inherits from require('stream').Stream, but does not have require('stream').Duplex in it's prototype chain, so the instanceof check fails and the stream is not correctly wrapped.

In later node versions, these lines were changed to the following:

https://github.com/nodejs/node/blob/be2cbccf003d110cad00317090072788021efa56/lib/_tls_wrap.js#L306-L311

Here, the instanceof check was dropped, and all objects that are not native net.Socket instances will be wrapped.

nodejs/node#12799 was the pull request that fixed the crash, and the bug report over in nodejs/node#3655 mentions the same error message, but with different preconditions.

I was thinking about how using Symbol.hasInstance in readable-stream could be used to monkeypatch the instanceof check, but Symbol.hasInstance is not available in Node v4. :(

@addaleax Any suggestions on how to workaround this issue? As it stands, duplexpair in combination with TLSSocket is broken on Node v4 and v6. 😞

[arthurschreiber]

@addaleax Maybe it would make sense if duplexpair just used the native stream module instead of readable-stream?

[arthurschreiber]

Closing this in favor of #738.

@joux3 Thank you so much for preparing these changes! ❤️