HTTP/2 ServerResponse.destroy() has the same effect as ServerResponse.end()

[szmarczak]

• Version: 14.11.0
• Platform: Linux solus 5.6.19-158.current #1 SMP PREEMPT Sun Jul 26 14:17:01 UTC 2020 x86_64 GNU/Linux
• Subsystem: http2

What steps will reproduce the bug?

const http = require('http2');

const server = http.createServer((request, response) => {
    response.write('a');
    
    setTimeout(() => {
		// Does the same as response.end()
        response.destroy();
    }, 100);
});

// HTTP/2
server.listen(() => {
    const session = http.connect(`http://localhost:${server.address().port}`);
    const stream = session.request();
    stream.resume();
    stream.end();

    stream.once('aborted', () => {
        console.log('stream aborted');
    });
    
    stream.once('end', () => {
        console.log('stream end');
        
        session.close();
        server.close();
    });
    
    stream.once('error', () => {
        console.log('stream error');
    });
});

// HTTP/1.1:
// server.listen(() => {
    // const request = http.get(`http://localhost:${server.address().port}`, response => {
	// 	console.log('got response');
	// 	response.resume();

    //     response.once('aborted', () => {
    //         console.log('aborted');
    //         server.close();
	// 	});

	// 	response.once('end', () => {
	// 		console.log('response end');
	// 	});

	// 	response.once('close', () => {
	// 		console.log('response close');
	// 	});
    // });
// });

Note: it works as expected when the method is e.g. POST and stream.end() is not called.

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

Always.

What is the expected behavior?

aborted or an error event.

What do you see instead?

end event is emitted.

[kanongil]

I just encountered this issue as well. You can workaround this issue by adding any error to the destroy() call.

Once you do that, you will trigger another bug in the client, where end is emitted before error:

stream end
stream error

[kanongil]

Calling destroy() without an error on an active stream should probably end up sending a RST_STREAM frame with code 8. to signal that it was aborted.

[kanongil]

Hmm, this is actually already tested against here (introduced in #15074):

node/test/parallel/test-http2-compat-serverresponse-destroy.js

Lines 44 to 51 in b15ed65

	{
	const req = client.request();
	req.on('response', common.mustNotCall());
	req.on('error', common.mustNotCall());
	req.on('end', common.mustCall());
	req.on('close', common.mustCall(() => countdown.dec()));
	req.resume();
	}

Except, the test is wrong! – and doesn't match the equivalent HTTP1 behaviour (which will emit an 'aborted' event in the client)

I tried messing with the _destroy() implementation in core.js, but when I add a NGHTTP2_CANCEL code to the close, the client will just ignore the code and treat it as a normal stream end! So I guess we are up to 3 bugs now...

Actually, the aborted comment is plain wrong, since the aborted event is only emitted when the writable side is still open (which it won't be). So this logic just loses all such aborts:

node/lib/internal/http2/core.js

Lines 2200 to 2204 in ff02801

	// RST code 8 not emitted as an error as its used by clients to signify
	// abort and is already covered by aborted event, also allows more
	// seamless compatibility with http1
	if (err == null && code !== NGHTTP2_NO_ERROR && code !== NGHTTP2_CANCEL)
	err = new ERR_HTTP2_STREAM_ERROR(nameForErrorCode[code] || code);

[kanongil]

FYI, I made an attempt at fixing all 3 bugs in #35209.