HTTP/2 request pending forever

[maxfliri]

Describe the bug

• Node.js version: 15.8.0
• OS & version: Mac OS Catalina 10.15.7

Actual behavior

When I send an http/2 request, got returns a response that remains in pending state forever, it never resolves nor rejects. This seems to happen when there is a problem with a network connection that was previously established successfully.

I have two failing test cases, reproduced below as a mocha test. The test sends some requests with { http2: true } to a stub http server controlled by the test itself. The failing tests are the second and the third.

Test case 2:

Interestingly this test passes when it's run on its own, but fails when I run all the tests. Given that the test runs after the first test, the behaviour is like this:

1. send an http/2 request to a running server, with a successful response
2. stop the server
3. send another request to the same server
4. got returns a promise that never resolves; the test eventually fails with a timeout

Test case 3:

This test always fails, even when run on its own.

1. send an http/2 request to a running server and wait for a response
2. the server closes the connection without sending back any data
3. the promise returned at step 1 never resolves; the test eventually fails with a timeout

Expected behavior

In both cases, the promise returned by got should be rejected with an appropriate error representing the underlying problem.

Code to reproduce

const assert = require("assert");
const got = require("got");
const http2 = require("http2");


describe("got", () => {
  const opts = {
    http2: true,
    timeout: { connect: 1000, secureConnect: 1000, lookup: 1000, socket: 1000, request: 1000, response: 1000, send: 1000 },
    throwHttpErrors: false,
    retry: 0,
    https: { rejectUnauthorized: false },
  };

  let http2Stub;
  const url = "https://localhost:56780";

  beforeEach(async () => {
    http2Stub = await Http2Stub.create(56780);
  });

  afterEach(async () => {
    await http2Stub.close();
  });

  // this test always passes
  it("returns an ok response", async () => {
    http2Stub.setupResponse(200);

    const res = await got(url, opts);

    assert.strictEqual(res.statusCode, 200);
  });

  // this tests passes when run on its own — when it runs after the previous one, got returns a promise that never
  // resolves, and the test fails with a timeout
  it("throws an exception when it cannot connect to the server", async () => {
    await http2Stub.close();

    await assert.rejects(() => got(url, opts), /connect ECONNREFUSED/);
  });

  // this tests always fails — here the server is setup to close the connection immediately after receiving it; got
  // returns again a promise that never resolves, and the test fails with a timeout
  it("throws an exception when connection to server is lost", async () => {
    http2Stub.setup(stream => stream.destroy());

    await assert.rejects(() => got(url, opts), /Stream prematurely closed/);
  });
});


class Http2Stub {

  static create(port) {
    const stub = new Http2Stub(port);
    return stub.start();
  }

  constructor(port) {
    this.port = port;
    this.sessions = [];
    this.server = http2.createSecureServer({ allowHTTP1: true, key: KEY, cert: CERT });
    this.server.on("session", session => this.sessions.push(session));
  }

  start() {
    return new Promise(resolve => this.server.listen(this.port, "localhost", () => resolve(this)));
  }

  setupResponse(status) {
    this.server.on("stream", (stream) => {
      stream.respond({ ':status': status });
      stream.end();
    });
  }

  setup(listener) {
    this.server.on("stream", listener);
  }

  close() {
    if (this.server.listening) {
      this.sessions.forEach(s => s.destroy());
      return new Promise((resolve, reject) => this.server.close(err => err ? reject(err) : resolve()));
    }
    return Promise.resolve();
  }
}


// self signed certificate and key - these are for test only and were generated specifically for this example

const CERT = '-----BEGIN CERTIFICATE-----\n' +
  'MIICpDCCAYwCCQDdgxsB6kB63jANBgkqhkiG9w0BAQsFADAUMRIwEAYDVQQDDAls\n' +
  'b2NhbGhvc3QwHhcNMjEwMjI0MTgxNjA4WhcNMzEwMjIyMTgxNjA4WjAUMRIwEAYD\n' +
  'VQQDDAlsb2NhbGhvc3QwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDS\n' +
  'AVq+mUn1NActQThFjv/G+/zeygRDjUzD+VZmfhqq9p01xnb8JCvhuZ93H9gYqhZF\n' +
  'cpYlcILvTi551FdYa+LzNicEJM8RH7vO+C7XmiXjEJnQYL3cuiyMsGM2OHE/vkWt\n' +
  'JJaQaT8JhoP/YUgJdrN8U0w/D2f8h9jMLWPD1FgornwH1VCuuoGGuZ9bji/KQw5N\n' +
  'iUaOsiGIKKEqgh0zIlYTEjuH20pRRT2181LQ1Oxw/ZSvAsXSXuU5Fg9rwJ/HKjOE\n' +
  'GjYuG5EsQDredHi1a+Am2O9UxFkcnQGaeN0UBnZtEH5mObRd3eWMnBAv0eeJkNal\n' +
  '8PFBRm2tE5UccMv9/6o9AgMBAAEwDQYJKoZIhvcNAQELBQADggEBAABeBP/WNZRe\n' +
  'B1AfbX7Sg/DuFnlzYJIr4wcxnlORDz1Vy+h6Wb7fRHRXp5SJ/jbGuI/oRUhpta1v\n' +
  '+37tTbGcRKyQhLEWFj8Pdh/XQ3Ug7qgbepsb3sDxcjRsn0OyFRYIpFkrpLBpdjv5\n' +
  'BpJA/yZ0hFgr7cix4KaoFJxGBktpqh3RKnX7qN5g9e2L0fewNy3NeLn7++P88DCs\n' +
  'dSL/tZfQVzLb+5BraExXDQZku3tpoOCQ8QHveueCyn5tVisvUGS6SfZXzEdmd3p2\n' +
  'nypRCcvmfiR/2j66HxsTTDzgtMXE0ZyF7GhVgM2pVDOwVc9uVP0xeIYkIGEq39Vt\n' +
  'z7ZccSSHzog=\n' +
  '-----END CERTIFICATE-----';

const KEY = '-----BEGIN RSA PRIVATE KEY-----\n' +
  'MIIEpAIBAAKCAQEA0gFavplJ9TQHLUE4RY7/xvv83soEQ41Mw/lWZn4aqvadNcZ2\n' +
  '/CQr4bmfdx/YGKoWRXKWJXCC704uedRXWGvi8zYnBCTPER+7zvgu15ol4xCZ0GC9\n' +
  '3LosjLBjNjhxP75FrSSWkGk/CYaD/2FICXazfFNMPw9n/IfYzC1jw9RYKK58B9VQ\n' +
  'rrqBhrmfW44vykMOTYlGjrIhiCihKoIdMyJWExI7h9tKUUU9tfNS0NTscP2UrwLF\n' +
  '0l7lORYPa8CfxyozhBo2LhuRLEA63nR4tWvgJtjvVMRZHJ0BmnjdFAZ2bRB+Zjm0\n' +
  'Xd3ljJwQL9HniZDWpfDxQUZtrROVHHDL/f+qPQIDAQABAoIBADnXm6nxyLgb+3oQ\n' +
  'g7JM/9BL6ctncyM0ERfNXmneg/Pg904veuhaAigrG2wRPlEU0AuS0x4+ziGhtBVk\n' +
  'UiaNmLYKjVaL2OjLh8wq+aPy1kqjcOo/KyxXrxuVHc56X18CRmi5MitWgcFa5pJ5\n' +
  'tgC9TBSLUO3xjV+1/xXFzrvKifmyDS9bRfIWba7zcKQa3JB+/7kOkex+C+JeEHJV\n' +
  'M+uEA/J34E3As899c5SUrE4s4A1p7qkyJ8B26WJbdFnrrwEDN2AB6ggqfuWp/B8e\n' +
  'w/omMs/qC9h4JxTsf6s3gevc9V9Z2rn6sxWgZCP3CffLRONM/+KL8ESUua472D9w\n' +
  'GSSTCMECgYEA/09na+VlAvFhrIXNA06ic3H6pXc8sAam6FFPjo0CxcSzFPn9+3TE\n' +
  '5qMl5crrC1blO4/DRtu1mCSJoY35qfq4RfDIIXsrdJ/go/0Zx9z2mco85QAXG2Is\n' +
  'gLqeJY33fUiHADTSbymzkiAJ7ng6BdTraoD7joBqwJMj5RAlNkmtU7ECgYEA0pKd\n' +
  'C3ELoz0Soe7SQO2oO1OePnRk4znNlZ1XDuauTKMDOdGRx9wZu2kyIle2iX7Q8MTP\n' +
  '0Ck9QBJPYS6oKGeJZNCcflNfMtN4OgPKM+rLAX3+/eoHHnvx9n14luKygMLctkkJ\n' +
  '7/qxZiAKwDtwusSS/1FdcSep76E9WxuXv49q3k0CgYEA+I0bCEWI8zZ/em/ASOny\n' +
  '6SUbeJ7+a/ft4dnW89Z/zn1SQqemBXmGf2pxaKcF8EImZLfuyjr3LSjU/Hy1hC/b\n' +
  '2esxSrcYdS94iO3MfXC2er4STnaqCDSpUqFbeQAe4s8K7r59507XzPh38rsE8cx5\n' +
  'a3Qqcm6+fsBAf64aLCHKJeECgYEAxORAIbWnGxB8/psPT5SorChom587wleHCnFf\n' +
  'ONire48k8ggp1oXQLbOUJBZ94JyKg8aTReF5mxJD1OvKYlVFW9XPrjMInb6r+RsY\n' +
  'E2lkPlXwer07wN5GBaOWgQchv1H1DCDJQPHYtFQbmVk68/fgNwl+ZNKgjCbo9uqa\n' +
  '/ov8cjUCgYBdczuZYebxTMUxChAEbn1dbqi6t9s25VF0zjz5YXbVlW9nO1b7+Bzi\n' +
  '7/jSBAZFKLqSpIk3kpObvkoWIm6UVYTkp0cmaqLe9bbjtrFz/43DK8fVLWiAxXny\n' +
  'n8quJyPvdNXfKE3/Q/x1piyX1gisgCIY5VQG/pArzs82rk062jg6nw==\n' +
  '-----END RSA PRIVATE KEY-----';

Checklist

• I have read the documentation.
• I have tried my code with the latest version of Node.js and Got.

[szmarczak]

HTTP/2 in Got currently is very buggy as it uses outdated version of http2-wrapper. I'll release a stable version today.

[szmarczak]

Ok, so everything is 100% finished but the docs. I'll do the docs later today and then release http2-wrapper@1.0.0.

[szmarczak]

Released http2-wrapper@1.0.0, you can apply this in Got via:

const http2wrapper = require('http2-wrapper');
const http2got = got.extend({
    request: http2wrapper.auto
});

Also we don't use mocha here. I'm closing this for now. If the problem still exists, I'll reopen.

[maxfliri]

Thanks for looking into this. I just gave this a try: it fixed case 2, but case 3 is still failing.

Sorry about mocha, it's what we use here: the code example was adapted from a test out of our codebase. What would you prefer instead?

[szmarczak]

Here at Got we use ava: https://github.com/avajs/ava

[szmarczak]

Have you tried Node.js 15.10.0?

[szmarczak]

Ok, so I've managed to reproduce the issue. Working on this rn.

[szmarczak]

So I've investigated and figured out the following. The test name "throws an exception when connection to server is lost" is invalid. What actually happens is this: the stream is closed with rstCode 0 before the HEADERS frame is received.

[maxfliri]

What do you mean by invalid? That test tries to simulate what happens if the connection to the server is lost while a message exchange is in progress, without a graceful termination, such as, for example, in case of crash of the server, or in case of sudden loss of network connectivity.

[szmarczak]

That test tries to simulate what happens if the connection to the server is lost while a message exchange is in progress

Well, in reality the doesn't do that.

[maxfliri]

I understand that the test may not simulate that condition perfectly. I'm quite new to HTTP2 and I still have a lot to learn about it. But I think the test it's still a valid one that simulates a condition that can happen in the wild: a server can close the connection at any time, without following the protocol. The client library should detect the fact and handle it, for example by throwing an exception.

[szmarczak]

Ok so I released 1.0.3 which is 100% the same beta version. The new version is released as 2.0.0. Sorry for the trouble. I had to do this because I didn't expect that the beta version specified in package.json would automatically bump up to 1.0.0.

[szmarczak]

I think the test it's still a valid one that simulates a condition that can happen in the wild

No, it's not valid because it doesn't do that. It just destroys a stream. And a stream is not a socket, the socket is still alive in this case. To test this properly, you'd have to destroy the entire server.

[szmarczak]

The client library should detect the fact and handle it, for example by throwing an exception.

It does. Check http2-wrapper@2.0.0.

[szmarczak]

To test this properly, you'd have to destroy the entire server.

Like: server.destroy(). Call session.destroy() on all sessions.

[maxfliri]

Brilliant! I upgraded to http2-wrapper 2.0.0, and now it works as expected.

a stream is not a socket, the socket is still alive in this case

Thanks for pointing this out, I see the mistake now. I misunderstood what stream.destroy() does.

To test this properly, you'd have to destroy the entire server. [...] Call session.destroy() on all sessions.

I added this test to my suite and it works too.

Thank you so much for your help, I really appreciate the effort you put in and how quickly you sorted this out!

[szmarczak]

No problem :) Thank you for the third case, it was a tricky one!