Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
skenqbx opened this issue Sep 23, 2015 · 11 comments
Closed

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

skenqbx opened this issue Sep 23, 2015 · 11 comments
Labels
confirmed-bug Issues with confirmed bugs. http Issues or PRs related to the http subsystem. net Issues and PRs related to the net subsystem.

Comments

@skenqbx
Copy link
Contributor

skenqbx commented Sep 23, 2015

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.

@brendanashworth brendanashworth added the http Issues or PRs related to the http subsystem. label Sep 23, 2015
@not-implemented
Copy link
Contributor

Same problem discovered here with node 4.3.1.

@raymondfeng
Copy link
Contributor

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

@jasnell jasnell added the confirmed-bug Issues with confirmed bugs. label Apr 9, 2016
@jasnell
Copy link
Member

jasnell commented Apr 9, 2016

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.

@jasnell jasnell added the net Issues and PRs related to the net subsystem. label Apr 9, 2016
@bnoordhuis
Copy link
Member

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

@indutny
Copy link
Member

indutny commented Apr 9, 2016

@bnoordhuis exactly

@cjihrig
Copy link
Contributor

cjihrig commented Apr 9, 2016

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

@indutny
Copy link
Member

indutny commented Apr 9, 2016

@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 mscdex removed the net Issues and PRs related to the net subsystem. label Apr 9, 2016
@mscdex mscdex added the net Issues and PRs related to the net subsystem. label Apr 13, 2016
@mscdex
Copy link
Contributor

mscdex commented Apr 13, 2016

@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
Copy link
Member

indutny commented Apr 13, 2016

@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
Copy link
Contributor

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 added a commit to indutny/io.js that referenced this issue Apr 19, 2016
This will provide `bytesRead` data on consumed sockets.

Fix: nodejs#3021
@indutny
Copy link
Member

indutny commented Apr 19, 2016

Should be fixed by #6284

MylesBorins pushed a commit that referenced this issue Apr 20, 2016
This will provide `bytesRead` data on consumed sockets.

Fix: #3021
PR-URL: #6284
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this issue Apr 21, 2016
This will provide `bytesRead` data on consumed sockets.

Fix: #3021
PR-URL: #6284
Reviewed-By: Ben Noordhuis <[email protected]>
joelostrowski pushed a commit to joelostrowski/node that referenced this issue Apr 25, 2016
This will provide `bytesRead` data on consumed sockets.

Fix: nodejs#3021
PR-URL: nodejs#6284
Reviewed-By: Ben Noordhuis <[email protected]>
jasnell pushed a commit that referenced this issue Apr 26, 2016
This will provide `bytesRead` data on consumed sockets.

Fix: #3021
PR-URL: #6284
Reviewed-By: Ben Noordhuis <[email protected]>
indutny added a commit to indutny/io.js that referenced this issue May 20, 2016
This will provide `bytesRead` data on consumed sockets.

Fix: nodejs#3021
PR-URL: nodejs#6284
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this issue Jun 1, 2016
This will provide `bytesRead` data on consumed sockets.

Fix: #3021
PR-URL: #6284
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this issue Jun 23, 2016
This will provide `bytesRead` data on consumed sockets.

Fix: #3021
PR-URL: #6284
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this issue Jun 24, 2016
This will provide `bytesRead` data on consumed sockets.

Fix: #3021
PR-URL: #6284
Reviewed-By: Ben Noordhuis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. http Issues or PRs related to the http subsystem. net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.