test: test server response that waits for request end

[awwright]

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

See #29609 for information.

[mscdex]

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

[BridgeAR]

@nodejs/http PTAL

[Trott]

@lpinca

[lpinca]

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]

@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]

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]

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

[awwright]

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

[awwright]

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]

@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]

@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]

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

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

[Trott]

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

[ronag]

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

[awwright]

@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]

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]

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]

@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]

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

[awwright]

Included in #29836