Response object socket & connection properties fix #878

Closed
wants to merge 6 commits into
from

Projects

None yet

4 participants

@neocoder
neocoder commented Apr 5, 2011

Hi guys,

I found that if browsers makes several http request on a single TCP connection, response object passed into the HTTP server's requestListener callback on a second request missed socket and connection properties.

Alex.

@koichik
koichik commented Apr 6, 2011

Hi,

Did you test it?
If assignSocket() was called when socket._httpMessage was not null, it will throw an AssertionError.
I think Node does NOT support HTTP pipelining.

@neocoder
neocoder commented Apr 6, 2011

Of course I tested it. :) I loaded a page with images (usual front page of a website) in google chrome and res.socket was undefined on some requests. It happens only on if the page loaded first time after server start. I refreshed the page with Сtrl-R to load it without the cache. And Node does support pipelining, KeepAlive is true by default.

This is the part of original function i made a change to. Read my comments

parser.onIncoming = function(req, shouldKeepAlive) {
    incoming.push(req);

    var res = new ServerResponse(req);   
        // creates new Response object in which .socket and .connection are not defined

    debug('server response shouldKeepAlive: ' + shouldKeepAlive);
    res.shouldKeepAlive = shouldKeepAlive;
    DTRACE_HTTP_SERVER_REQUEST(req, socket);

    if (socket._httpMessage) {
      // socket._httpMessage is a pointer to last "res" object in the queue

      // There are already pending outgoing res, append.
      outgoing.push(res);

          // we appending *newly* created "res" object to the outgoing queue
          // but it doesn't have .socket and .connection properties as they are
          // both set with res.assignSocket(socket) call.
    } else {
      res.assignSocket(socket);
    }

    // So socket assigned to the request only if it's the only request on the connection

    // Then we do some checks and emit request event
        self.emit('request', req, res);
@koichik
koichik commented Apr 6, 2011

You should write a test code.
example:

var net = require('net'), http = require('http'), assert = require('assert').ok;
var gotError = false;

var server = http.createServer(function(req, res) {
});
server.on('connection', function(client) {
  client.on('end', function() {
    server.close();
  });
});
server.on('clientError', function(e) {
  gotError = true;
  console.log(e);
});
server.listen(8080, '127.0.0.1', function() {
  var socket = net.createConnection(8080, '127.0.0.1');
  socket.on('connect', function() {
    socket.write('GET / HTTP/1.1\r\n');
    socket.write('Host: localhost\r\n\r\n');
    socket.write('GET / HTTP/1.1\r\n');
    socket.write('Host: localhost\r\n\r\n');
    socket.end();
  });
});

process.on('exit', function() {
  assert(!gotError);
});

Run the test on debug mode (enable assersions).

$ ./configure --debug
$ make clean all
$ ./node_g /tmp/test.js
assert.js:81
  throw new assert.AssertionError({
        ^
AssertionError: true == false
    at ServerResponse.assignSocket (http.js:336:3)
    at HTTPParser.onIncoming (http.js:1075:11)
    at HTTPParser.onHeadersComplete (http.js:108:31)
    at Socket.ondata (http.js:1007:22)
    at Socket._onReadable (net.js:679:27)
    at IOWatcher.onReadable [as callback] (net.js:177:10)
@koichik
koichik commented Apr 6, 2011

And, here is test for pipelining.
v0.4.5 is ok (You are right, Node supports pipelining).
But apply your patch, failed (client received only one response).

var net = require('net'), http = require('http'), assert = require('assert');
var gotError = false;
var count = 0;

var server = http.createServer(function(req, res) {
  var id = ++count;
  setTimeout(function() {
    res.writeHead(200, {'Content-Type': 'text/plain'});
    res.end('id: ' + id);
  }, (3 - count) * 1000);
});
server.on('clientError', function(e) {
  gotError = true;
});
server.listen(8080, '127.0.0.1', function() {
  var socket = net.createConnection(8080, '127.0.0.1');
  socket.setEncoding('utf8');
  socket.on('connect', function() {
    socket.write('GET / HTTP/1.1\r\n');
    socket.write('Host: localhost\r\n\r\n');
    socket.flush();
    socket.write('GET / HTTP/1.1\r\n');
    socket.write('Host: localhost\r\n\r\n');
    socket.flush();

    var buf = '';
    socket.on('data', function(data) {
      buf += data;
    });
    socket.on('end', function() {
      server.close();
      var lines = buf.split('\r\n').filter(function(s) {
        return s.indexOf('id: ') === 0;
      });
      assert.equal(lines.length, 2);
      assert.equal(lines[0], 'id: 1');
      assert.equal(lines[1], 'id: 2');
    });

    setTimeout(function() {
      socket.end();
    }, 3000);
  });
});

process.on('exit', function() {
  assert.ok(!gotError);
});
@neocoder
neocoder commented Apr 7, 2011

Thanks, you are right, I should made a test first. I modified your test a bit and added it to all other tests.
And I made a proper patch. I didn't recognize the _flush() method call in the res.assignSocket() at first time.

I had another issue on my production server. It happens very rarely and I don't know how to write a test here. Here's the commit neocoder/node@6cc840b. What do you think?

@koichik
koichik commented Apr 8, 2011

Hi,

I prefer:

  if (this.connection &&
      this.connection._httpMessage === this &&
      this.connection.writable) {
    // There might be pending data in the this.output buffer.
    while (this.output.length) {
      var c = this.output.shift();
      var e = this.outputEncodings.shift();
      this.connection.write(c, e);
      if (!this.connection.writable) {
        this._buffer(data, encoding);
        return false;
      }
    }

    // Directly write to socket.
    return this.connection.write(data, encoding);

I think that the problem occurred in Socket.prototype._writeOut():

  try {
    bytesWritten = this._writeImpl(buffer, off, len, fd, 0);
    DTRACE_NET_SOCKET_WRITE(this, bytesWritten);
  } catch (e) {
    this.destroy(e);
    return false;
  }

The socket is no longer writable when an error occurred here.
But I can't write this test. It's difficult to produce the error when last data of res.output was sent.

@trevnorris

Ran supplied test (after removing lines 7,8,25,28) on v0.8.14 and the test passes. Think this now a non-issue.

@bnoordhuis
Member

Yes, I'm pretty sure this is fixed (it had better be by now). Closing.

@bnoordhuis bnoordhuis closed this Nov 20, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment