win,test: fix test-tls-over-http-tunnel

[piscisaureus]

This patch fixes an ECONNRESET error on Windows by:

• Fixing the spelling of the 'Connection' and 'Proxy-Connection'
  headers in the proxy response.
• Adding a 'Connection: keep-alive' header to the request that gets
  sent through the tunnel, so the receiving end doesn't destroy the
  connection after the transaction is complete.

In addition this patch replaces some uses of Socket.destroy() by the
slightly more graceful destroySoon(), which ensures that all write
buffers are fully flushed before destroying a connection.

@tellnes I'm not sure if I worded the commit message correctly. Can you review it?

R=@tellnes
R=@rvagg
R=@iojs/platform-windows

[domenic]

I wonder why we were exhibiting different behavior on Unix vs. Windows in the first place, even for a messed-up test?

[tellnes]

Proxy-Connection is not documented in any standard documents and there is no other references to it in the code base than this test, so why not just remove it?

I'm not a native english speaker, so I'm probably not the best person to review commit messages, but it looks fine to me.

[piscisaureus]

@domenic I don't know. Strangely enough this test was originally written on windows by @igorzi so it likely passed at some point. But it seems that something in the tls or http implementations has changed such that a connection is now closed (as it should) if the connection: keep-alive header isn't set.

@indutny Maybe knows more...

@tellnes I noticed you were the last person touching this test so I added you as a reviewer. If you don't know, nevermind.

[tellnes]

When I touched this test I did notice it needed some love and planned to to that after that pr was landed, but I've forgotten that since then.

But I still think we should remove every mention of Proxy-Connection in this test. It is only confusing when you are trying to understand what the test does.

[piscisaureus]

But I still think we should remove every mention of Proxy-Connection in this test

Done, updated. Thanks for the suggestion.

[domenic]

We should probably file a follow-up bug regarding the observable behavior differences between platforms? Or is this low-level enough that we don't anticipate people noticing?

[seishun]

Well, the test passes for me now. Can't say anything about the validity of the changes though.

[indutny]

LGTM, with a follow-up bug for investigating the behavior.

[piscisaureus]

@domenic, @seishun, @indutny
Even if I think this patch makes the test better, there clearly is something wrong with the 1.4.x series wrt TLS behavior. Let's punt on landing this patch until we figure it out.

[piscisaureus]

@indutny Can you prioritize investigating this? I can't figure out why this happens.
I think we should revert the streamwrap patches until we know what's up.

[piscisaureus]

@indutny I got as far as figuring out that one of the sockets is closed (e.g. uv_close() is called) while there are still 2 write requests pending. At the same time this condition is true, so somehow the "pending callback" counter isn't maintained correctly in node.

[piscisaureus]

It turns out that this test catches a serious issue. Therefore let's not change the test.

[indutny]

@piscisaureus do we have a follow-up issue where I can comment?

[indutny]

IFNG (Information for the Next Generation): this is an issue related to the TLSWrap implementation itself, nothing to do with StreamBase changes.