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

http2: trailers or response to be emitted? #41251

Closed
szmarczak opened this issue Dec 20, 2021 · 8 comments
Closed

http2: trailers or response to be emitted? #41251

szmarczak opened this issue Dec 20, 2021 · 8 comments
Labels
http2 Issues or PRs related to the http2 subsystem. linux Issues and PRs related to the Linux platform.

Comments

@szmarczak
Copy link
Member

szmarczak commented Dec 20, 2021

Version

v17.3.0

Platform

Linux pop-os 5.15.5-76051505-generic #202111250933~1638201579~21.04~09f1aa7-Ubuntu SMP Tue Nov 30 02: x86_64 x86_64 x86_64 GNU/Linux

Subsystem

http2

What steps will reproduce the bug?

const http2 = require('http2');

const server = http2.createServer((request, response) => {
    response.writeContinue();
    response.end(); // Make it end('') and it emits `response` instead
});

server.listen(() => {
    const { port } = server.address();

    const session = http2.connect(`http://localhost:${port}`);
    const request = session.request({
        ':path': '/',
        ':method': 'GET',
    });

    request.on('response', (headers) => {
        console.log('got response', headers);
    });

    request.on('headers', () => {
        console.log('got headers');
    });

    request.on('trailers', (trailers) => {
        console.log('got trailers', trailers);
    });

    request.end();
    request.resume();

    request.on('end', () => {
        session.close();
        server.close();
    });
});

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

Always.

What is the expected behavior?

got headers
got response [Object: null prototype] {
  ':status': 200,
  date: 'Mon, 20 Dec 2021 08:27:48 GMT',
  [Symbol(nodejs.http2.sensitiveHeaders)]: []
}

What do you see instead?

got headers
+got trailers [Object: null prototype] {
  ':status': 200,
  date: 'Mon, 20 Dec 2021 08:27:48 GMT',
  [Symbol(nodejs.http2.sensitiveHeaders)]: []
}

Additional information

I'm not sure if this is the expected behavior. HTTP/1 for comparison (trailers are empty):

    const http = require('http');

    const server = http.createServer((request, response) => {
        response.writeContinue();
        response.end();
    });

    server.listen(() => {
        const { port } = server.address();

        const request = http.get(`http://localhost:${port}`, response => {
            response.resume();

            response.on('end', () => {
                console.log(response.trailers, response.rawTrailers);

                server.close();
            });
        });

        request.on('information', () => {
            console.log('information');
        });
    });
information
{} []
@szmarczak
Copy link
Member Author

@szmarczak
Copy link
Member Author

szmarczak commented Dec 20, 2021

Wireshark

trailers event:

Screenshot from 2021-12-20 10-08-23

Screenshot from 2021-12-20 10-13-11

response event:

Screenshot from 2021-12-20 10-09-54

Screenshot from 2021-12-20 10-14-23

@szmarczak
Copy link
Member Author

The last frame in the sequence bears an END_STREAM flag, noting that
a HEADERS frame bearing the END_STREAM flag can be followed by
CONTINUATION frames that carry any remaining portions of the header
block.

Trailing header fields are carried in a header block that also
terminates the stream. Such a header block is a sequence starting
with a HEADERS frame, followed by zero or more CONTINUATION frames,
where the HEADERS frame bears an END_STREAM flag.

👍

@szmarczak
Copy link
Member Author

Following: then

const http2 = require('http2');

const server = http2.createServer((request, response) => {
    response.end(); // just empty end, nothing else
});

server.listen(() => {
    const { port } = server.address();

    const session = http2.connect(`http://localhost:${port}`);
    const request = session.request({
        ':path': '/',
        ':method': 'GET',
    });

    request.on('response', (headers) => {
        console.log('got response', headers);
    });

    request.on('headers', () => {
        console.log('got headers');
    });

    request.on('trailers', (trailers) => {
        console.log('got trailers', trailers);
    });

    request.end();
    request.resume();

    request.on('end', () => {
        session.close();
        server.close();
    });
});
got response [Object: null prototype] {
  ':status': 200,
  date: 'Mon, 20 Dec 2021 09:47:15 GMT',
  [Symbol(nodejs.http2.sensitiveHeaders)]: []
}

I think it should emit trailers instead of response.

Wireshark

Screenshot from 2021-12-20 10-45-43

Screenshot from 2021-12-20 10-45-54

@Mesteery Mesteery added the http2 Issues or PRs related to the http2 subsystem. label Dec 20, 2021
@iam-frankqiu iam-frankqiu added the linux Issues and PRs related to the Linux platform. label Dec 21, 2021
@MoonBall
Copy link
Member

MoonBall commented Jan 5, 2022

I think it should emit trailers instead of response.

I think HTTP/2 should be consistent with HTTP/1 behavior. About the above case, they shouldn't emit trailers event.

@szmarczak
Copy link
Member Author

@MoonBall Please note that in the original issue it emits trailers instead of response. I'd prefer consistent behavior over making it look like HTTP/1.

@MoonBall
Copy link
Member

MoonBall commented Jan 5, 2022

In the original issue I think it should emit response. I would like to dig into it

@aduh95
Copy link
Contributor

aduh95 commented May 11, 2024

Duplicate of #38258

@aduh95 aduh95 marked this as a duplicate of #38258 May 11, 2024
@aduh95 aduh95 closed this as not planned Won't fix, can't repro, duplicate, stale May 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http2 Issues or PRs related to the http2 subsystem. linux Issues and PRs related to the Linux platform.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants