Enable socket reuse

[mgorczyca]

Existing code checks req.useChunkedEncodingByDefault, which prevents socket reuse for persistent connection.

[simov]

@mgorczyca the tests in the request module seems to be passing with this fix, however I'm a bit wary about adding fixes without tests. And since this repository doesn't have any, can you add a failing test in the request repository instead? We have a test-agent.js file or something similar there.

[mgorczyca]

@simov the error which led to this PR occurred when attempting to use a SOAP https client that required authentication via login. Because the forever-agent code currently does not reuse the socket in the case of https and PUT, the login worked but the authentication did not persist. So, the next command (after login) could not execute.

The simpler test case we have been using is to see that there is only one connect in the strace output after doing two PUTs on an https URL. Seeing two connects in the strace output indicates that the socket is not being reused, but does not result in an error.

Given the context of the issue as described, what are your thoughts on the type of test case that would mesh well with the tests you already have? Thanks!

[mgorczyca]

@simov I have added a test for this fix in the request repository, in the test-agent.js file as you suggested.

[robinjmurphy]

@mgorczyca @simov Is there any reason why useChunkedEncodingByDefault was used over chunkedEncoding originally? This is proving to be an issue for us as we're using Node 0.10.0 and want to avoid costly TLS handshakes for PUT requests.

Happy to add some tests to this module directly for the behaviour if there's no obvious reason to stick with useChunkedEncodingByDefault.

[simov]

@robinjmurphy, sure, feel free to add tests in request, as for why useChunkedEncodingByDefault was used - I have no idea, it's like that from the start. Probably @mikeal could shed some light on that.

[robinjmurphy]

I started to write a basic test, but on further investigation it looks like req.chunkedEncoding is always equal to false at the point at which it's evaluated. When making a chunked POST request, it's not until the response event fires on the req object that the chunkedEncoding property is set to true.

I think this means that including the && !req.chunkedEncoding has no effect and suggests that the presence of !req.useChunkedEncodingByDefault is simply a means of limiting socket reuse to GET, HEAD, DELETE, OPTIONS and CONNECT requests (see _http_client.js.)

I haven't yet looked at the core pooling code in Node v4, but if keep alive is disabled for POST/PUT requests there, maybe there's a reason for it that we're missing?