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

HTTP/2 server.close() is broken with "allowHTTP1" mode #51493

Closed
JoshuaWise opened this issue Jan 16, 2024 · 1 comment · Fixed by #51569
Closed

HTTP/2 server.close() is broken with "allowHTTP1" mode #51493

JoshuaWise opened this issue Jan 16, 2024 · 1 comment · Fixed by #51569

Comments

@JoshuaWise
Copy link

JoshuaWise commented Jan 16, 2024

Version

v21.6.0

Platform

Darwin Kernel Version 19.6.0

Subsystem

none

What steps will reproduce the bug?

Run the script below:

const fs = require('fs');
const https = require('https');
const http2 = require('http2');

const USE_COMPATIBILITY_MODE = true;

(async function main() {
	const sockets = new Set();
	let server;

	if (USE_COMPATIBILITY_MODE) {
		server = http2.createSecureServer({
			allowHTTP1: true,
			cert: fs.readFileSync('test-cert.pem'),
			key: fs.readFileSync('test-key.pem'),
		});
	} else {
		server = https.createServer({
			cert: fs.readFileSync('test-cert.pem'),
			key: fs.readFileSync('test-key.pem'),
		});
	}

	server.on('connection', (socket) => {
		sockets.add(socket);
		socket.on('close', () => sockets.delete(socket));
	});
	server.on('request', (req, res) => {
		console.log('request received');
		const message = 'hello world.';
		res.setHeader('Content-Length', String(Buffer.byteLength(message)));
		res.setHeader('Content-Type', 'text/plain; charset=utf-8');
		res.writeHead(200);
		res.end(message);
	});

	await new Promise(resolve => server.listen(443, resolve));
	console.log('Listening at https://localhost:443');

	await openKeepAliveSocket();
	console.log(`closing ${sockets.size} socket(s)...`);
	server.close(() => console.log('done'));
})();

function openKeepAliveSocket() {
	return new Promise((resolve, reject) => {
		const req = https.get('https://localhost:443', {
			rejectUnauthorized: false,
			headers: { connection: 'keep-alive' },
		});
		req.on('error', reject);
		req.on('response', (res) => {
			console.log('response status ' + res.statusCode);
			res.on('error', reject);
			res.on('data', () => {});
			res.on('end', () => {
				console.log('response ended');
				resolve();
			});
		});
	});
}

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

I can reliably reproduce it 100% of the time.

What is the expected behavior? Why is that the expected behavior?

The expected behavior is for the HTTPS server to close down immediately (as soon as the request ends). This is what happens when using https.createServer(), as demonstrated by changing USE_COMPATIBILITY_MODE to false in the script above.

What do you see instead?

Instead, the HTTPS server stays open for 5 seconds (which is the default keep-alive duration).

In other words, calling server.close() properly shuts down idle "keep-alive" sockets when using the http or https modules, but not when using the http2 module with allowHTTP1: true.

Additional information

You'll need a TLS certificate to run the script above. I created one by following this guide in the Node.js documentation to create a self-signed certificate.

@xsbchen
Copy link
Contributor

xsbchen commented Jan 26, 2024

I creating a PR for this

xsbchen added a commit to xsbchen/node that referenced this issue Jan 26, 2024
xsbchen added a commit to xsbchen/node that referenced this issue Jan 30, 2024
xsbchen added a commit to xsbchen/node that referenced this issue Jan 31, 2024
nodejs-github-bot pushed a commit that referenced this issue Feb 1, 2024
Fixes: #51493
PR-URL: #51569
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
rdw-msft pushed a commit to rdw-msft/node that referenced this issue Feb 9, 2024
Fixes: nodejs#51493
PR-URL: nodejs#51569
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
targos pushed a commit that referenced this issue Feb 15, 2024
Fixes: #51493
PR-URL: #51569
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
marco-ippolito pushed a commit to marco-ippolito/node that referenced this issue Feb 19, 2024
Fixes: nodejs#51493
PR-URL: nodejs#51569
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
richardlau pushed a commit that referenced this issue Mar 25, 2024
Fixes: #51493
PR-URL: #51569
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
richardlau pushed a commit that referenced this issue Mar 25, 2024
Fixes: #51493
PR-URL: #51569
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
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

Successfully merging a pull request may close this issue.

2 participants