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

Cannot reuse a TLS socket for a HTTP2 session #33343

Closed
szmarczak opened this issue May 10, 2020 · 13 comments
Closed

Cannot reuse a TLS socket for a HTTP2 session #33343

szmarczak opened this issue May 10, 2020 · 13 comments

Comments

@szmarczak
Copy link
Member

szmarczak commented May 10, 2020

What steps will reproduce the bug?

const http2 = require('http2');
const tls = require('tls');

const options = {
  ALPNProtocols: ['h2'],
  host: 'nghttp2.org',
  servername: 'nghttp2.org',
  port: 443,
  settings: {}
};

tls._connect = tls.connect;

/*
tls.connect = (...args) => {
    console.log(...args, args[1].toString());
    return tls._connect(...args);
};
*/

const socket = tls._connect(options, () => {
    console.log('Connected!');
    const session = http2.connect('https://nghttp2.org', {
        createConnection: () => socket
    });

    session.once('remoteSettings', () => {
        console.log('Received remote settings!');
    });
});

How often does it reproduce? Is there a required condition?

Always.

What is the expected behavior?

Connected!
Received remote settings!

What do you see instead?

Connected!

Additional information

Reference: #16256

@rickyes
Copy link
Contributor

rickyes commented May 11, 2020

The latest code does not reproduce this issue.

@szmarczak
Copy link
Member Author

What latest code? Do you mean Node.js master branch?

@rickyes
Copy link
Contributor

rickyes commented May 11, 2020

Yes, it may have been fixed in some commit, I didn't reproduce this issue in macOS v10.14.

@szmarczak
Copy link
Member Author

Indeed, it's fixed on the master branch. Feel free to close the issue, but I'd prefer to keep this open until there's a release that fixes it.

@szmarczak
Copy link
Member Author

Closing since a fix has been released a while ago.

@pimterry
Copy link
Member

I ran into this too. I've done some quick tests with the repro above, and:

  • I can reproduce this in the latest Node 12 (12.18.3)
  • I can't reproduce this in Node 14.3+. It fails in 14.1 & 14.2, but seems to be fixed from that point onwards. I've specifically tested 14.3, 14.6 and 14.8 (the latest release).

Doing some digging through the changelog, looks like this is the fix: #33209.

That hasn't yet been backported to v12 though. It'd be great if that could be included in the next v12 release to get this fixed, since it causes some big problems for lots of interesting HTTP/2 use cases (like detecting HTTP/2 support during TLS setup, as in https://github.com/szmarczak/http2-wrapper/). I'm not sure where to get started with making that happen, but maybe @addaleax can help?

@fenying
Copy link

fenying commented Aug 19, 2020

Same here, I tested v8.17.0, v10.22.0, v12.18.3, v14.8.0, and only v12.18.3 could reproduce this problem.

see my reproduce: https://github.com/fenying/nodejs-bug-code-h2-binding

@szmarczak

@grantila
Copy link

This is a really serious bug, could it please be opened up? cc @addaleax

@grantila
Copy link

Especially serious since 12 is LTS. My fetch-h2 package, which is relatively well used is totally broken because of this.

@addaleax
Copy link
Member

@grantila Correct me if I’m wrong, but based on the conversation above, backporting #33209 to v12.x would fix this, right? I’ll open a backport PR as suggested over there, there isn’t really anything to do here in the issue anymore. If there’s still problems with backporting, feel free to help with debugging those

@addaleax
Copy link
Member

#34859

@addaleax
Copy link
Member

@grantila Also, I might be wrong, but if the fix works, then socket.on('secureConnect', () => socket.secureConnecting = false); should be a functional workaround.

@grantila
Copy link

grantila commented Aug 20, 2020

I'll be damned, it does work @addaleax, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants