http/server: request.socket.bytesRead always 0 for IncomingMessage

[skenqbx]

Tested with node.js v4.1.0 & v4.1.1 (v0.10.x does not have this issue)

See the following test (parallel/test-http-bytesread.js) to reproduce the issue;

'use strict';
var common = require('../common');
var assert = require('assert');
var http = require('http');

var body = 'hello world\n';
var sawEnd = false;
var bytesReadServer = 0;
var bytesReadClient = 0;

process.on('exit', function() {
  assert(sawEnd);
  console.log('ok');
});

var httpServer = http.createServer(function(req, res) {
  httpServer.close();

  req.on('readable', function() {
    var data = req.read();
    if (data !== null) {
      bytesReadServer += data.length;
    }
  });

  req.on('end', function() {
    assert(bytesReadServer > 0);
    assert(req.socket.bytesRead > 0);

    sawEnd = true;
    res.writeHead(200, { 'Content-Type': 'text/plain' });
    res.end(body);
  });
});

httpServer.listen(common.PORT, function() {
  var req = http.request({
    method: 'PUT',
    port: common.PORT
  }, function(res) {
    res.on('readable', function() {
      var data = res.read();
      if (data !== null) {
        bytesReadClient += data.length;
      }
    });

    res.on('end', function() {
      assert(bytesReadClient > 0);
      assert(res.socket.bytesRead > 0);
    });
  });

  var chunk = new Array(1024 + 1).join('7');
  var bchunk = new Buffer(chunk);

  for (var i = 0; i < 1024; i++) {
    req.write(chunk);
    req.write(bchunk);
    req.write(chunk, 'hex');
  }

  req.end();
});

I could not yet figure out what causes this, any help is appreciated.

[not-implemented]

Same problem discovered here with node 4.3.1.

[raymondfeng]

@bnoordhuis Can you take a look? I need to use bytesRead/bytesWritten for analytics.

[jasnell]

ping @bnoordhuis @trevnorris ... a quick check on this indicates that in the test case above, it does not appear that the onread method in lib/net.js is being called for some as yet unknown reason. Because it is not called, the bytesRead is never incremented.

[bnoordhuis]

I think it's because of #2355 - the HTTP parser short-circuits the socket infrastructure in lib/net.js now. /cc @indutny.

[indutny]

@bnoordhuis exactly

[cjihrig]

If this is expected behavior, what are the next steps for this issue? Remove bytesRead? Fix it so that bytesRead works again?

[indutny]

@cjihrig I don't think that there is an efficient way to fix it... I'd rather vote for its removal. Probably need to discuss it on @nodejs/ctc call

[mscdex]

@indutny What were some of the possible solutions you had in mind? @trevnorris said something about adding a data listener and manually incrementing the value from inside there. Does a simple fix like that impose a significant performance regression?

[indutny]

@mscdex I don't think I have any efficient solutions in mind. One is about making this property a getter for the case of consumed sockets and increment counter manually in C++.

[trevnorris]

The point about adding the data listener was only to illustrate that it could be fixed, but not that that is how it should be fixed.

I personally like the idea of a getter. That has the lowest overhead for the common case of nit checking the property.

[indutny]

Should be fixed by #6284