big bug for http-agent keep alive??

[windyrobin]

I found it doesn't work ,
well ,maybe my test way isn't correct , but please just check keep-alive feature again
I think the test file : test-http-keep-alive.js isn't so valuable.

my test code :

client:

var count = 10;
var agent = new http.Agent({"maxSockets" : 3});
var options = {
host: 'localhost',
  port: 3458,
  path: '/',
  method: 'GET',
  agent : agent,
  headers : {"Connection" : "keep-alive"}
};

function looptest(){
 var req = http.request(options, function(res){
    res.on("end", function(){
        console.log("count : " + count);
        if(--count <= 0){ 

         }else{
            process.nextTick(function(){
            looptest();
          }); 
        }   
    }); 
}); 
 req.end();
}

looptest();

the server is writen by me ,and when it got a connection it will print a line
the js test result is 10 lines:

get a connection
get a connection
get a connection
get a connection
get a connection
get a connection
get a connection
get a connection
get a connection
get a connection

I think it should print only one line ,since we use keep-alive and only one socket is used!!!!!!

so I check it with siege (keep-alive mode)

 siege -c 1 -r 10 localhost:3458/

the result is one line :

get a connection

why??

[peritus]

This looks like it behaves like it should: The connection is kept alive. What did you expect it to do ?

[windyrobin]

@peritus

because we use agent in js client, so just only one socket/connection is needed.
I expect the js client should only print "one" line ,,
just as siege test

[bnoordhuis]

Can you post the server code?

[johnlettman-old]

It is entirely possible that those extra connections are for automatic resources such as the favicon. Browsers make these calls without being invoked.

[johnlettman-old]

Oh, my bad, you're using a HTTP utility to test it.

[windyrobin]

@bnoordhuis

the server is a cluster writen by my own :
https://github.com/windyrobin/iCluster/blob/master/tcpWorker.js

when a worker received a socket handle posted by master process ,
it will print "get a connection" (I remove this debug info in github code ,so you couldn't see it)

[windyrobin]

@bnoordhuis
worker code is just like that :

function onhandle(self, handle){
  if(self.maxConnections && self.connections >= self.maxConnections){
    handle.close();
    return;
  }
  var socket = new net.Socket({
    handle : handle,
    allowHalfOpen : self.allowHalfOpen 
  });
  socket.readable = socket.writable = true;
  socket.resume();
  self.connections++;
  socket.server = self;
  self.emit("connection", socket);
  socket.emit("connect");
  console.log("get a connection");
}

 server = http.createServer(function(req, res){
    var r, i;
    for(i=0; i<10000; i++){
      r = Math.random();
    }
    res.writeHead(200 ,{"content-type" : "text/html"});
    res.end("hello,world");
    child_req_count++;
  });

process.on("message",function(m ,handle){
    if(handle){
      onhandle(server, handle);
    }
    if(m.status == "update"){
      process.send({"status" : process.memoryUsage()});
    }
  });

[bnoordhuis]

You don't set a Content-Length or Transfer-Encoding header.

[windyrobin]

@bnoordhuis

when I write code like below in server:

res.writeHead(200 ,{"content-type" : "text/html"});
res.end("hello,world");

nodejs will automatically add 'transfer-encoding': 'chunked'

I inspect the response headers in client :

{ 'content-type': 'text/html',
  connection: 'keep-alive',
  'transfer-encoding': 'chunked' }

so I think there is another hidden bug caused this problem

[bnoordhuis]

Right, that's true. The bug is in your test though: http.Agent only keeps the connection alive if there are pending requests. That is never the case in your test, a new request is submitted only after the previous one has finished.

[windyrobin]

@bnoordhuis

Besides that ,could you please give some tips on how to make http-keep-alive requests,
I don't want want the agent.socket be destroied when there is no pending request ,then I could
reuse the socket again in some mill-seconds later...

I think the current implement of Agent has the pitfall ,maybe it should expose a property like :

agent.maxKeepAliveTime 

just as

agent.maxSockets 

if user sets it to a number less than 0 ,it will be persistent,
maybe we should set it default to 3-5 seconds , I think this's a good parameter for most cases.

for common http request ,we use global.agent ,which is a special instance of Agent
we set
global.agent.maxSockets = 5
global.maxKeepAliveTime = 0 //close immediately when we finish a request

then we are compatible with the old api

[bnoordhuis]

No. Keeping idle connections around just because you can is bad Internet neighbourship. If you want to do that, either implement your own http.Agent class or keep the request queue filled.

[windyrobin]

@bnoordhuis

I have added some words...

for common http request ,we use global.agent ,which is a special instance of Agent
we set

global.agent.maxSockets = 5
global.maxKeepAliveTime = 0 //close immediately when we finish a request

then we are compatible with the old api

for Nodejs is mostly used as a middle-ware ,
so I think a good http-poll module is really really neccesary and really really import ...
otherwise the users have to implement a new one...

[atimb]

If anyone needs a "real" keep-alive implementation, I shared mine: (not yet perfect... but worked for my problem, and you can improve :)
https://gist.github.com/2963672

@bnoordhuis That is untrue. You can equally be a bad internet neighbor with the current implementation by spamming TCP SYN / FIN (if you send requests one-by-one to an endpoint -- you need the response to construct the next request for example -- this might be a more common use case than you think).
Can you argue, why browsers do employ this strategy (ie. keeping idle connections), but node.js shouldn't ?

[bnoordhuis]

Can you argue, why browsers do employ this strategy (ie. keeping idle connections), but node.js shouldn't ?

I don't know about your browser but my FF 13 install doesn't keep the connection open if there's nothing to do.

Re: keep-alive, we may add a one tick respite, i.e. don't close the connection until the next tick of the event loop. That gives people time to queue a new request in the 'end' listener of the current request.

[atimb]

I don't know about your browser but my FF 13 install doesn't keep the connection open if there's nothing to do.

I have just done a quick test with FF 13 and Chrome 19 on Mac, opened an url, and checked the TCP connections with netstat. It seems to me with both browsers the connections stay in ESTABLISHED state for around a minute, by default.

[garo]

We just ran into problems with the current implementation. In our case, our node.js processes do a http query from Amazon DynamoDB Database. Our servers had tens of thousands of TCP connection in TIME_WAIT state, so they're actually preventing new connections in the entire server.

The http requests are so fast that even with out server doing about 200 requests per second, there's practically no time when the requests will be queued with the http client so that keep-alive would be used.

[atimb]

I have a basic keep-alive agent implementation, but it may be useful to start out to solve your case: https://gist.github.com/atimb/2963672

[CrabDude]

The "bad neighbourship" argument is relative. Leaving unnecessary connections open is wasteful and closing them is probably a good default, but constantly closing and opening connections (quickly consuming available sockets on the remote server by leaving them in a TIME_WAIT state) can also be "bad neighbourship."

I think this should be configurable.

FWIW, I'm aware that there is a related discussion of what to do with request-less HTTP errors resulting from disconnects, but I don't see why that prevents optional socket reuse.