`remoteSettings` event is not emitted on a HTTP/2 session when given a socket with some data

[szmarczak]

• Version: v15.1.0
• Platform: Linux solus 5.6.19-159.current #1 SMP PREEMPT Fri Oct 16 17:49:06 UTC 2020 x86_64 GNU/Linux
• Subsystem: http2

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
};

const socket = tls.connect(options, async () => {
    console.log('Connected!');
    
    await new Promise(resolve => setTimeout(resolve, 1000));
    
    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!
HTTP2 13382: Http2Session client: setting up session handle
HTTP2 13382: Http2Session client: sending settings
HTTP2 13382: Http2Session client: submitting settings
HTTP2 13382: Http2Session client: created
HTTP2 13382: Http2Session connect [Function: createConnection]
(node:13382) Warning: Setting the NODE_DEBUG environment variable to 'http2' can expose sensitive data (such as passwords, tokens and authentication headers) in the resulting log.
(Use `node --trace-warnings ...` to show where the warning was created)
+HTTP2 13382: Http2Session client: new settings received
+Received remote settings!
HTTP2 13382: Http2Session client: settings received

What do you see instead?

szm@solus ~/Desktop/http2-wrapper $ export NODE_DEBUG=http2 && node demo.js 
Connected!
HTTP2 13357: Http2Session client: setting up session handle
HTTP2 13357: Http2Session client: sending settings
HTTP2 13357: Http2Session client: submitting settings
HTTP2 13357: Http2Session client: created
HTTP2 13357: Http2Session connect [Function: createConnection]
HTTP2 13357: Http2Session connect: injecting 40 already in buffer
(node:13357) Warning: Setting the NODE_DEBUG environment variable to 'http2' can expose sensitive data (such as passwords, tokens and authentication headers) in the resulting log.
(Use `node --trace-warnings ...` to show where the warning was created)
HTTP2 13357: Http2Session client: settings received

Additional information

Related with #35678 and #35475

[szmarczak]

/cc @mmomtchev

[szmarczak]

	await new Promise(resolve => setTimeout(resolve, 1000));

	console.log(session.remoteSettings);

If we add the piece above then we'll see that it has received the settings, but didn't emit the remoteSettings event.

[mmomtchev]

Yes, I did mention it in the comments of the PR, but the PR was already approved and it stayed like this
Everything works beside the remote settings event
Making the connection work was trivial and sending the remote settings event required moving some code around and as this was one of my first PRs in Node I didn't do it

[szmarczak]

Yes, I did mention it in the comments of the PR

Hmm... I think I'm blind because I cannot find it... Or was that a different PR?

[szmarczak]

Making the connection work was trivial and sending the remote settings event required moving some code around and as this was one of my first PRs in Node I didn't do it

Cool. Thanks anyway :) Greatly appreciated.

[szmarczak]

https://github.com/mmomtchev/node/blob/e36b31c1bc4292085e10d2f019a1a046076cfc2c/src/node_http2.cc#L1411

[szmarczak]

Most likely

https://github.com/mmomtchev/node/blob/e36b31c1bc4292085e10d2f019a1a046076cfc2c/src/node_http2.cc#L1408-L1409

fails which means that data is received before any listener is attached.

Seems like

https://github.com/mmomtchev/node/blob/e36b31c1bc4292085e10d2f019a1a046076cfc2c/lib/internal/http2/core.js#L3132-L3141

should be wrapped in a process.nextTick(() => { ... }).

[szmarczak]

This proves that's right:

https://runkit.com/szmarczak/5fa47f71dbabb1001b64a492

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

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

const socket = tls.connect(options, async () => {
    console.log('Connected!');
    
    await new Promise(resolve => setTimeout(resolve, 1000));
    
    const data = socket.read();
    
    const session = http2.connect('https://nghttp2.org', {
        createConnection: () => socket
    });
    
    const kHandle = Object.getOwnPropertySymbols(session).find(x => x.toString() === 'Symbol(kHandle)');
    
    process.nextTick(() => {
        session[kHandle].receive(data);
    });

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

[szmarczak]

I'll make a PR.

[mmomtchev]

@szmarczak clean my console.log from the unit test while you are at it

[szmarczak]

already did :)