Unexpected socket hang up triggered by http.ClientRequest

[c4milo]

I've been bending HTTP a bit to implement a bidirectional communication that works within the scope of RFC2616 without using websockets nor the known polling techniques. While doing that in node 0.6.11, I ran into an unexpected socket hang up, triggered by http.ClientRequest. I've been wondering whether it's something that has to be fixed in http.ClientRequest or I definitely would have to use the net module to write my own client.

I wrote some code that reproduces the issue in https://gist.github.com/1889610

FWIW, I had it working before in nodejs 0.4.12, with similar code, without any issues.

Any help is very much appreciated.

[c4milo]

BTW, this is the output if I run:
$ NODE_DEBUG="net http" node client.js

NET: connect: find host localhost
HTTP: write ret = false
NET: afterConnect
HTTP: write ret = true
NET: Drain the connect queue
NET: destroy
NET: close
{ [Error: socket hang up] code: 'ECONNRESET' }
HTTP: HTTP socket close
socket closed

I don't understand why the socket is getting destroyed since I am not closing or destroying the connection in client.js.

[bnoordhuis]

Your example is working as I would expect it to in v0.6.x. You need to pass a custom http.Agent object to http.request(), the default one closes the connection when there are no requests queued up.

[OrangeDog]

Where does one get a custom http.Agent from? The documentation does not say.

[bnoordhuis]

http://nodejs.org/docs/latest/api/http.html#http.Agent

Subclass it, then pass it in the options argument to http.request().

[OrangeDog]

Yes, but you shouldn't have to look through the source code to see what you can override.
Also, new http.Agent({closeIdleConnections: true, etc.}) would be a better interface than writing a subclass.

[bnoordhuis]

Yes, but you shouldn't have to look through the source code to see what you can override.

Documentation patches are welcome.

[c4milo]

Hey @bnoordhuis, why if I put the first write, on socket connection, within an interval function of zero ms, it does work as expected without a custom Agent? See please client.js gist .

[c4milo]

I also disagree with the current default behavior since it does not honor rfc2616.

[bnoordhuis]

Hey @bnoordhuis, why if I put the first write, on socket connection, within an interval function of zero ms, it does work as expected without a custom Agent?

Because the connection is still open at that point, it's closed at the next tick of the event loop.

[c4milo]

hmm, I am still not convinced. If you change the 0 for 1000 ms it will still work as expected. As I said, it does not honor rfc2616, http.ClientRequest should not close a keep-alive connection at all, unless the user directly invokes .end(). It's like if a browser closes the connection deliberately while getting the chunked response from the server because there are not more requests queued up.

[c4milo]

@mikeal any opinions here?

[tamzinblake]

@c4milo long rfc is long. Maybe it would help folks if you pointed to the bit that's being violated.

[c4milo]

http://www.w3.org/Protocols/rfc2616/rfc2616-sec8.html > 8.1.2.1 Negotiation

[tamzinblake]

@c4milo No violation there. There is only one 'MUST' in that section, and it does not pertain to your complaint. Also, there are 2 'SHOULD's and 1 'SHOULD NOT', and those don't seem to be violated either. As far as I can see, servers can throw away keep-alives to their hearts' content.

Remember that 'MAY' is there specifically so that vendors can establish reasonable behavior that works for their unique situations. So it's entirely up to Node to decide whether this is a reasonable default, without in any way failing to honor the RFC.

[c4milo]

I've been digging more, it seems like there is a race condition, req.write() writes the first chunk before the actual request. It might be a misusage of the API from my part. But I sort of believe that it will be possible to send the request, either in the next tick after socket onconnect event is emitted or along with the first write attempt to the socket.

I created another gist with the Wireshark output of the tcp stream that shows what I just said.
https://gist.github.com/1893548

[mikeal]

it is incorrect to claim that we do not follow the RFC. we can and do re-use the connection for requests in the future when we have them. The RFC does not require clients to hold connections open indefinitely.

The current default behavior exists for a reason. It is unreliable to maintain connections indefinitely because a connection can be assigned to a new request and after it was closed by the remote end (waiting RTT for the message to be received it was closed), which will fail that request for reasons unknown (where normally we would assume the server either crashed or we sent something invalid).

This has been discussed long and hard by many, many people. It has led to a feature in request called forever:

https://github.com/mikeal/request/blob/master/forever.js
https://github.com/mikeal/request/blob/master/main.js#L729

You can use this to hold a minimum number of sockets open for an indefinite amount of time and it will retry any abnormally failed connections.

You should be able to req.write() before nextTick() and have that data get cached until after the socket is assigned and connected. If not that is a different bug, please log it.

This bug can probably be closed.

[c4milo]

oh god, finally a more comprehensive explanation. Thank you @mikeal.

This makes complete sense to me:

The current default behavior exists for a reason. It is unreliable to maintain connections indefinitely because a connection can >be assigned to a new request and after it was closed by the remote end (waiting RTT for the message to be received it was >closed), which will fail that request for reasons unknown (where normally we would assume the server either crashed or we >sent something invalid).

However, I think the issue is happening because I am attempting to write a chunk too soon in the lifecycle of the http request. It isn't even http.Agent but an invalid HTTP conversation that is causing the socket to hang up.

Regarding:

You should be able to req.write() before nextTick() and have that data get cached until after the socket is assigned and >connected. If not that is a different bug, please log it.

I am attempting to write when socket.on('connect') is emitted. I shouldn't be doing that? Should I wait a bit for the request to be sent first by http.ClientRequest? is there any event being emitted by http.ClientRequest to let me know when the headers were sent?

Here is where I am attempting to make the first req.write:
https://gist.github.com/1889610#file_client.js L33

Sorry for being so insistent, I am just trying to understand what's going on.

[mikeal]

If the Agent doesn't have a free socket and it's not over the maxSocket limit it will create a new object and assign it. The actual assignment happens in a nextTIck() but as you can probably tell a socket will only ever be assigned to a ClientRequest object long before it emits "connect" or long after (it is reused out of the pool).

Now, writing to a ClientRequest object before socket assignment may be a bug, but it would have to happen before nextTick().

[c4milo]

So yeah, @mikeal, my attempting to req.write() is being done when the socket is assigned and connected. But, the chunk is being written before the request and headers. Will that qualify as an issue for you? If so, I'll create a new issue.

[mikeal]

@c4milo can you paste the exception you're seeing so that I can tell which line is emitting/throwing it?

[c4milo]

@mikeal #2812 (comment)

[c4milo]

you can also test, in node 0.6.11, using https://gist.github.com/1889610 (client and server)

[mikeal]

ok, yeah, you're writing before nextTick(), this may be a bug.

Related, I've been talking to @isaacs about modifications to the Stream interface. I've proposed that all streams are created in a pause() state and must be resumed later. This would solve the problem we see here and unify the handling of writes that happen before actual file descriptors are open and available. As it stands now we have special handling in each stream.

[c4milo]

@mikeal cool, I am going to create a new issue then.

Regarding the modification you've been proposing to @isaacs, it sounds interesting. It will be cool also, to document or draw the event model or state machine of those things. So that people can understand better how the messaging happens between different abstractions (i.e: http and net modules). And even within the same module.