@grpc/grpc-js & grpc.max_metadata_size

[hugebdu]

Is your feature request related to a problem? Please describe.

In order to migrate from grpc to grpc-js as suggested I need to be able to specify gprc.max_metadata_size, which isn't supported currently.

If I understand correctly, the default of 64k is implied by the core http2 module and supporting it seems to be pretty easy

[murgatroid99]

Unfortunately, it's not that simple. According to my team, that setting you linked is just advisory. The other libraries need to do explicit enforcement of that option and I can't find anything in the Node http2 docs about restricting the size of received header blocks, so implementing this may require a change to Node itself.

[hugebdu]

well in our case we need to increase it actually.
I tried sending trailers with metadata size of > 32K using @grpc/grpc-js and the response never gets back to the client.

[murgatroid99]

That sounds like a bug. A stream hanging like that is never an intended outcome, even if there is a violation of that kind of restriction.

[hugebdu]

@murgatroid99 so on my machine the threshold is ~64K, not 32K - my bad.
test to reproduce

[murgatroid99]

That is interesting. I don't know what might cause that. One potentially important thing to note there is that that parameter that you are using is for sending trailing metadata. You can send headers using the call.sendMetadata method.

[hugebdu]

Yup, tried that as well. Same result

[murgatroid99]

OK, I tried changing the value of the setting that you linked to on both the client and the server, and that did not change the behavior of the test. So that's definitely not the right solution here.

I think the next step is to create the same test using only Node's built in http2 module. If that has the same behavior, then this is definitely a bug in Node itself, and an issue should be filed there.

[hugebdu]

I played with that setting as well.
will try to find some time and check the http2 layer apart. will update.
10x

[hugebdu]

Michael, hey.

Spent some time investigating further and got some findings.
Here you can find the raw http2-only reproduction. And apparently adding maxSendHeaderBlockLength to ServerOptions solves the problem. Also verified with the original test.

Do you think you can now support gprc.max_metadata_size propertly?

Thanks

[murgatroid99]

Now that you have a reproduction using only the http2 library, it is clearly a Node bug, so you should file an issue against Node itself. In the meantime, I can set that option internally to a large number to work around the bug.

As I mentioned in my previous comment, the grpc.max_metadata_size option is for enforcing the upper limit, so it's not relevant to fixing this.

The documentation for maxSendHeaderBlockLength says

Attempts to send headers that exceed this limit will result in a 'frameError' event being emitted and the stream being closed and destroyed.

If you set that option and then exceed it, do you get the 'frameError' event or does it still just hang?

[hugebdu]

Michael, I don't think there's an issue with node's http2.

The server indeed emits frameError event, which contains the stream id among other info.

As for the client side - client only gets the close event.

Probably @grpc-js should listen to the session's frameError event (somewhere here?) and send back to the client one of the appropriate status codes - (DATA_LOSS, INTERNAL, OUT_OF_RANGE - you name it)

[hugebdu]

UPD
well, since it happens in this scenario upon trailers frame, looks like it is too late to send appropriate status code

Error [ERR_HTTP2_HEADERS_SENT]: Response has already been initiated.
    at ServerHttp2Stream.respond (internal/http2/core.js:2504:13)

Then maybe just adding a log/error message?
IMO still better than ignoring frameError completely.

WDYT?

[murgatroid99]

Wait, the client gets the close event? That changes everything. The grpc-js client handles the close event, and it should be ending the call when that happens.

[hugebdu]

The client does get close event eventually. Might be due to mocha test end run - need to verify that.
However the original test with grpc client still halts, so something isn't right anyway.

[hugebdu]

Michael, added debug printouts into call-stream.ts#attachHttp2Stream events and I don't get the close event in my halting test

Also tried adding frameError event handler into server-call.ts#Http2ServerCallStream constructor that simply destroys the stream

        this.stream.on('frameError', () => {
          this.stream.destroy();
        })

and now I don't see any halts any more, instead I get an error

Error: 13 INTERNAL: Received RST_STREAM with code 0
    at Object.callErrorFromStatus (node_modules/@grpc/grpc-js/src/call.ts:81:24)
    at Object.onReceiveStatus (node_modules/@grpc/grpc-js/src/client.ts:334:36)
    at Object.onReceiveStatus (node_modules/@grpc/grpc-js/src/client-interceptors.ts:434:34)
    at Object.onReceiveStatus (node_modules/@grpc/grpc-js/src/client-interceptors.ts:397:48)

probably can be improved with some other error code?

[murgatroid99]

The frameError documentation says

If the 'frameError' event is associated with a stream, the stream will be closed and destroyed immediately following the 'frameError' event. If the event is not associated with a stream, the Http2Session will be shut down immediately following the 'frameError' event.

Closing and destroying the stream should trigger the 'close' event. If that is not happening, that is a bug in Node.

[murgatroid99]

After doing my own testing, I have filed nodejs/node#35133. I will also figure out a good setting for maxSendHeaderBlockLength to avoid this issue in the future.

[hugebdu]

@murgatroid99 thanks!