HTTP2: API for checking if http2ServerRequest had body frame.

[sogaani]

Is your feature request related to a problem? Please describe.
Some express middleware check if httpRequest has/had body. e.g body-parser.
And it depends on content-length header.
Although a http request with body in http/1 must contain content-length header, http2 request with body can have no content-length header.
We can use http2ServerRequest.stream.readable only before readable stream ended to check if request might have body frame. I'm not sure how to check it after readable stream ended with current http2 compatibility APIs.

Describe the solution you'd like
I'd like to add new API to check if request had body, or readable stream had been read data after readable stream ended.

Describe alternatives you've considered
I notice that http2ServerRequest.stream._readableState.sync can be used. However we should not use _readableState.

[addaleax]

@nodejs/http2

[jasnell]

If the Http2Stream instance emits the 'end' event before any 'data' or 'readable' events are emitted, then there were no data frames and no payload.

[sogaani]

@jasnell It's true, but I concerned following case.

const express = require('express');

const app = express();
app.use((req, res, next) => {
    let data = '';
    req.on('data', (chunk) => {data += chunk});
    req.on('end', next);
});

app.use((req, res, next) => {
    // How to know req had body or not without other middle ware information?
});

[dougwilson]

Sounds like Express will go down the route to reaching into the Node.js internals since there doesn't seem to be a solution here.

[apapirovski]

@dougwilson We can probably add something if that's the trade-off. Let me think about a good API and get back to you.

[mcollina]

I don’t understand this issue. If we are missing an API we can add it, but I do not understand what is missing and what you need. What code are you trying to make work on both?

[apapirovski]

@mcollina Checking if the incoming request has or had a body (data event), with the limitation that this could happen at any time in the lifecycle of the request.

[jasnell]

At this point, there's really nothing to reach in to. Let's step back and examine how the code works...

In the method onSessionHeaders in lib/internal/http2/core.js, if the NGHTTP2_FLAG_END_STREAM flag is set, we end the readable side of the Http2Stream duplex because we know at that point there will be no data frames.

From that point on, the JS code will not know if there are any data frames until the underlying StreamBase pushes each chunk out, triggering the data and readable events (depending on which model you're using). DATA frames can arrive at any time after the stream is established and there is no guarantee at all that DATA frames will arrive before the middleware is invoked... so in the example..

app.use((req, res, next) => {
  // We really don't know if there's going to be a body or not unless the `Stream` is closed
});

The most we can do at this point is provide a flag that says whether or not any DATA frames have been received. Unless the streams readable side is closed, we cannot say for sure if DATA frames will be received at all.

[dougwilson]

In the OP I think @sogaani describes the use case pretty well. He found that http2ServerRequest.stream._readableState.sync works to detect the side effects of what we need and that's in an Express PR. I didn't want to use an internal API even though it works if it's possible not to, though. We don't need to know if any DATA frames arrived or not; only if they are going to arrive at all for a given request. This is mainly in the compat API. The last header frame contains the flag to indicate this, but we don't seem to have direct access to the frame flags in the compat API.

[jasnell]

We don't need to know if any DATA frames arrived or not; only if they are going to arrive at all for a given request. This is mainly in the compat API. The last header frame contains the flag to indicate this, but we don't seem to have direct access to the frame flags in the compat API.

By the time the middleware is invoked, we might not be able to reliably know if a DATA frame is going to arrive at all for a given request. All we would know for sure is whether it's possible for DATA frame to arrive. For instance, a client could send the request HEADERS frame followed immediately by a trailing HEADERS frame, without any DATA frames at all. Or, the stream could be closed with an RST_STREAM at any time. Or, the client can send an empty DATA frame with no payload and the end-stream flag set... and could do so well after the middleware is invoked.

[dougwilson]

We'll just accept the PR using .sync for now, then 👍

[jasnell]

btw, I'm not saying that we shouldn't add an API here, I'm trying to figure out what kind of API we can add that will be useful for the actual use case. Specifically, there are two things that I absolutely know we can do:

1. We can provide an indication that yes, DATA frames have been received or,
2. We can provide an indication that yes, it is possible for DATA frames to still be received.

What we cannot do reliably is reliably determine if DATA frames will or will not arrive for any given stream.

[dougwilson]

whether it's possible for DATA frame to arrive.

Right, that's what was said we wanted to know in the middleware.

[dougwilson]

We need both (1) and (2).

[dougwilson]

We need to equiv of what you can do in http1 to know if the request will have a body (it doesn't matter if the client craps out and never sends it) in a way we can check even after the data stream has been fully read. It's just a flag on the last http2 header frame that indicates if there can be data frames or not. All header frames arrive before the compat api invokes your function so we will have that info.

[mcollina]

I do not think http2ServerRequest.stream._readableState.sync should be used, and it is likely not going to be reliable in various situations. That flag is totally unrelated to HTTP2.

I think we really need to surface this info from the lower layers.

[jasnell]

excellent, that's the clarification I was hoping for. For 2, the easy way to determine if DATA frames can still be received is to just check if the readable side is still open. For 1, I'm thinking that a generic hadData boolean on the base readable stream API would suffice.

[dougwilson]

We don't need both (1) and (2), it can just be one signal. Maybe having both will be useful to others. The data we need is just a flag on the last headers frame.

[dougwilson]

Ex this is the exact question the code is trying to answer:

For a given http2 compact api request object at any part of it's life cycle after it was emitted to a request listener, did the HEADER (or last CONTINUATION frame) have the END_STREAM flag set or not?

I hope that is specific enough.

[jasnell]

You can already determine that by checking the readable state of the stream. If the end stream flag is set on the header, the readable side of the Http2Stream is closed immediately, which means if that flag is set, the stream will not be readable when it is passed to the request listener. If the stream is readable at that point, then that flag was not set and DATA frames might show up.

[dougwilson]

You can't tell in certain cases ex #22497 (comment)

[dougwilson]

i.e if a middleware fully read the stream before my code, it it then always have the readable false. This is the OP issue.

[dougwilson]

Remember my example said "object at any part of it's life cycle after it was emitted to a request listener"

[dougwilson]

How can I clarify further so we can stop talking in circles?

[jasnell]

I think we're on the same page now. This really isn't an HTTP2 specific issue. It's a readable streams issue. There needs to be a better way of determining after the fact that a readable ended with or without data. There doesn't need to be a http2 specific API here. I think @mcollina is going to work on a PR.

[dougwilson]

Even if streams did that, we'd still have to look into that private .sync method, because when the 'request' event listener fires and we get the req object, it will always be readable = true since the .end() waits until the next tick to actually end the stream (that's basically what the .sync is indicating).

The other thing to keep in mind is we need the exact answer to the question the code is trying to ask as defined above. That means that it's important to know if there was a zero-length DATA frame or not as the only DATA frame. If the readable doesn't consider that emitted data, then the readable streams fix wouldn't work in that case, either.

[mcollina]

Even if streams did that, we'd still have to look into that private .sync method, because when the 'request' event listener fires and we get the req object, it will always be readable = true since the .end() waits until the next tick to actually end the stream (that's basically what the .sync is indicating).

So, basically you need to know:

a. is data in the buffer
b. is the stream going to end in the next tick

The other thing to keep in mind is we need the exact answer to the question the code is trying to ask as defined above. That means that it's important to know if there was a zero-length DATA frame or not as the only DATA frame. If the readable doesn't consider that emitted data, then the readable streams fix wouldn't work in that case, either.

If that is the question, I do not understand how checking for .sync would help at all.

[dougwilson]

What I need to know is #22497 (comment)

[jasnell]

PR: #22843