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

test: test server response that waits for request end #29649

Closed
wants to merge 1 commit into from

Conversation

awwright
Copy link
Contributor

This is a test that, by all indications, should pass, and would have passed prior to 206ae31

See #29609 for information.

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 22, 2019
@mscdex
Copy link
Contributor

mscdex commented Sep 22, 2019

Subsystem prefix in the commit message should be test instead of http.

@BridgeAR
Copy link
Member

@nodejs/http PTAL

@Trott
Copy link
Member

Trott commented Sep 24, 2019

@lpinca

@Trott Trott added the http Issues or PRs related to the http subsystem. label Sep 24, 2019
@lpinca
Copy link
Member

lpinca commented Sep 24, 2019

This needs investigation. I cannot reproduce with net.Socket.

'use strict';

const assert = require('assert');
const http = require('http');

const testData = 'Hello, World!\n';

const server = http.createServer(function(req, res) {
  res.statusCode = 200;
  res.setHeader('Content-Type', 'text/plain');
  res.end(testData);
  req.once('end', function() {
    res.end(testData);
  });
});

server.listen(8080, function() {
  const req = http.request({ port: 8080 }, function(res) {
    res.setEncoding('utf8');

    let data = '';

    res.on('data', function(chunk) {
      data += chunk;
    });

    res.on('end', function() {
      assert.strictEqual(data, testData);
      server.close();
    });
  });

  req.end();
});

Works as expected.

@awwright
Copy link
Contributor Author

@lpinca Your test ends the response twice, this hangs for me when I remove the first call to res.end:

'use strict';

const assert = require('assert');
const http = require('http');

const testData = 'Hello, World!\n';

const server = http.createServer(function(req, res) {
  res.statusCode = 200;
  res.setHeader('Content-Type', 'text/plain');
  // res.end(testData);
  req.once('end', function() {
    res.end(testData);
  });
});

server.listen(8080, function() {
  const req = http.request({ port: 8080 }, function(res) {
    res.setEncoding('utf8');

    let data = '';

    res.on('data', function(chunk) {
      data += chunk;
    });

    res.on('end', function() {
      assert.strictEqual(data, testData);
      server.close();
    });
  });

  req.end();
});

@awwright
Copy link
Contributor Author

Actually, that's because the request was never resumed, ugh

req.resume();

seems to make it work again

The bug I'm looking to reproduce calls the request "end" event, but fails to write the ending to the response.

@awwright
Copy link
Contributor Author

I'm going to spend some time better adapting #29609

@awwright
Copy link
Contributor Author

Added a chunked test case that passes, and a Content-Length test case that does not (but clearly should)

@awwright
Copy link
Contributor Author

In chunked mode, Node.js works as expected (in all versions).

In Content-Length mode, Node.js fails:

Prior to 206ae31, Node.js failed only serverSide.on('end', common.mustCall());.

Starting 206ae31, Node.js fails both clientSide.on('end', common.mustCall()); and serverSide.on('end', common.mustCall());

@lpinca
Copy link
Member

lpinca commented Sep 27, 2019

@awwright I copied your tests and again I can't reproduce with net.Socket.

Details
const assert = require('assert');
const http = require('http');

const testData = 'Hello, World!\n';

{
  const server = http.createServer(function(req, res) {
    req.setEncoding('utf8');

    let data = '';

    req.on('data', function(chunk) {
      data += chunk;
    });

    req.on('end', function() {
      assert.strictEqual(data, testData);

      res.statusCode = 200;
      res.setHeader('Content-Type', 'text/plain');
      res.write(testData);
      res.end();
    });
  });

  server.listen(function() {
    const req = http.request(
      {
        port: server.address().port,
        method: 'PUT',
        headers: { Connection: 'close' }
      },
      function(res) {
        res.setEncoding('utf8');

        let data = '';

        res.on('data', function(chunk) {
          data += chunk;
        });

        res.on('end', function() {
          assert.strictEqual(data, testData);
          server.close();
        });
      }
    );

    req.write(testData);
    req.end();
  });
}

{
  const length = String(testData.length);

  const server = http.createServer(function(req, res) {
    assert.strictEqual(req.headers['content-length'], length);
    req.setEncoding('utf8');

    let data = '';

    req.on('data', function(chunk) {
      data += chunk;
    });

    req.once('end', function() {
      assert.strictEqual(data, testData);

      res.statusCode = 200;
      res.setHeader('Content-Type', 'text/plain');
      res.setHeader('Content-Length', testData.length);
      res.write(testData);
      res.end();
    });
  });

  server.listen(function() {
    const req = http.request(
      {
        port: server.address().port,
        method: 'PUT',
        headers: {
          Connection: 'close',
          'Content-Length': length
        }
      },
      function(res) {
        assert.strictEqual(res.headers['content-length'], length);

        res.setEncoding('utf8');

        let data = '';

        res.on('data', function(chunk) {
          data += chunk;
        });

        res.on('end', function() {
          assert.strictEqual(data, testData);
          server.close();
        });
      }
    );

    req.write(testData);
    req.end();
  });
}

@awwright
Copy link
Contributor Author

@lpinca I assume Socket is doing some work that avoids a race condition; I was able to recreate the bug with only "stream", so it's not even related to http, see #29609 (comment)

Nonetheless, is my assessment correct that this test should pass?

@lpinca
Copy link
Member

lpinca commented Sep 29, 2019

Nonetheless, is my assessment correct that this test should pass?

Yes, that's why in #29649 (comment) I said this needs investigation.

@ronag ronag mentioned this pull request Oct 3, 2019
4 tasks
@mscdex mscdex changed the title http: test server response that waits for request end test: test server response that waits for request end Oct 3, 2019
@Trott
Copy link
Member

Trott commented Oct 4, 2019

@awwright Any chance that applying #29836 will cause the test to pass?

@ronag
Copy link
Member

ronag commented Oct 4, 2019

@Trott: The commit and test from this PR is included in #29836

@awwright
Copy link
Contributor Author

awwright commented Oct 4, 2019

@Trott I suppose #29649 (that seems to be included in #29836) would pass, but makeDuplexPair is also implemented in and at least a couple packages: https://yarnpkg.com/en/package/duplexpair https://yarnpkg.com/en/package/native-duplexpair

(edit) Actually this would fix "tls", to whatever extent that's impacted

@awwright
Copy link
Contributor Author

awwright commented Oct 4, 2019

I didn't realize this was the test case I wrote up, let's try this again...

#29836 is a decedent commit of this PR so it seems to cause the test to pass; but few other libraries still use the old code; e.g. https://yarnpkg.com/en/package/native-duplexpair; and it would need to be updated in the Writable documentation why I need to handle an empty write as a special case.

Also, historical versions of my package would continue to fail, even though it was written for Node.js 12.0.0. Not the end of the world, but it makes e.g. git blame somewhat more difficult.

@ronag
Copy link
Member

ronag commented Oct 4, 2019

Writable documentation why I need to handle an empty write as a special case

Updated PR with doc changes. Please take a look and see if you have any further suggestion.

@awwright
Copy link
Contributor Author

awwright commented Oct 4, 2019

@ronag It seems unintuitive to me, but with that explanation (that _read isn't really a callback, more like an event, so note a special case where it doesn't get called), I don't really have any reason to object.

@ronag
Copy link
Member

ronag commented Oct 4, 2019

@awwright: I didn't quite follow that :). If you make a commit or PR I'm more than happy to cherry pick your suggestion.

Trott pushed a commit that referenced this pull request Oct 6, 2019
If nothing is buffered then _read will not be called and the
callback will not be invoked, effectivly deadlocking.

Fixes: #29758

PR-URL: #29836
Refs: #29649
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Trott pushed a commit that referenced this pull request Oct 6, 2019
PR-URL: #29836
Refs: #29758
Refs: #29649
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
@awwright
Copy link
Contributor Author

awwright commented Oct 8, 2019

Included in #29836

@awwright awwright closed this Oct 8, 2019
BridgeAR pushed a commit that referenced this pull request Oct 9, 2019
If nothing is buffered then _read will not be called and the
callback will not be invoked, effectivly deadlocking.

Fixes: #29758

PR-URL: #29836
Refs: #29649
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
BridgeAR pushed a commit that referenced this pull request Oct 9, 2019
PR-URL: #29836
Refs: #29758
Refs: #29649
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants