feature(grpc-js): Add possibility to provide maxSessionMemory http2 option through ChannelOptions

[dwrip]

Currently there is no way to set the parameter maxSessionMemory for http2.createServer() and http2.connect() options. In case when queued data at session is become more than default value there thrown exception.

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Andrey Melnik (7837e8e)

[murgatroid99]

What is the exception that you are seeing, and where is it getting thrown?

[dwrip]

What is the exception that you are seeing, and where is it getting thrown?

Thrown Error: 8 RESOURCE_EXHAUSTED

I found that this has already happened (issue #1158) and it looks like the problem was not solved

[murgatroid99]

What specific error details did you get with the RESOURCE_EXHAUSTED error? And how did you conclude that the solution was to increase the maxSessionMemory?

[Steuls]

What specific error details did you get with the RESOURCE_EXHAUSTED error? And how did you conclude that the solution was to increase the maxSessionMemory?

I have also had this error occur, and if you look at the source files the only source of this specific error code is "http2 Enhance your calm" This is directly related to the maxSessionMemory.

[dwrip]

What specific error details did you get with the RESOURCE_EXHAUSTED error? And how did you conclude that the solution was to increase the maxSessionMemory?

For example, if the server responds with a data (
in docs is "the current number of Http2Stream sessions, the current memory use of the header compression tables, current data queued to be sent, and unacknowledged PING and SETTINGS frames are all counted towards the limit"
) size larger than the default value of 10 megabytes of http2 module, there will be thrown an exception NGHTTP2_ENHANCE_YOUR_CALM.

they even have a test for that: https://github.com/nodejs/node/blob/606df7c4e79324b9725bfcfe019a8b75bfa04c3f/test/sequential/test-http2-max-session-memory.js

and this exception is wrapped into

grpc-node/packages/grpc-js/src/call-stream.ts

Line 550 in b78e5a0

case http2.constants.NGHTTP2_ENHANCE_YOUR_CALM:

where thrown RESOURCE_EXHAUSTED

I increased the maxSessionMemory value to 50 megabytes since my service can send data closely to this size and these errors no longer appear for me.

[murgatroid99]

OK, I see how this can help, but I am not sure about this exact solution. In particular, this would be the first time that we add a channel argument that does not correspond to a channel argument in the C core library. I am not sure we want to use the same naming scheme for it that we use for the others.

[dwrip]

Yes I agree with you. It is more likely that this parameter is specific to node.js http2 implementation along with other parameters that are not related to the grpc core channels implementation.

What do you think, maybe if extend parameters and do something like

private options: ChannelOptions & http2.SessionOptions;

constructor(options?: ChannelOptions & http2.SessionOptions) {
  this.options = options ?? {};
}

....

Object.keys(this.options)
  .filter(key => !key.startsWith('grpc.'))
  .forEach(key => serverOptions[key] = this.options[key]);

if ('grpc.max_concurrent_streams' in this.options) {
  serverOptions.settings = {
    maxConcurrentStreams: this.options['grpc.max_concurrent_streams'],
  };
}

....

[murgatroid99]

That would be excessive. There are a lot of options that shouldn't be modified, and I don't want to check all of the other options to solve this one problem.

[dwrip]

Why you need to check them? This options related to node http2 module optionals session settings and if someone need to change them then it means they know what they are doing and for what. :)

[murgatroid99]

The basic issue is that gRPC can have more consistent behavior if it has control over the options it uses to create the underlying http2 sessions, and can therefore predict the behavior of those sessions. It may be the case that none of those available options actually change the behavior badly, but I would have to do some work to verify that before I am willing to allow those options to be changed. That's the "checking" I was referring to. And of course if any of those options are actually incompatible with what gRPC is doing, code would be needed to filter them out.

In addition, while I'm willing to make an ad-hoc change to add one option, adding a whole class of options that I don't directly control is a change that I would want to put through a design review process.

[dwrip]

Ok. I understand.
There is another try:

in channel options interface you have additional field

grpc-node/packages/grpc-js/src/channel-options.ts

Line 39 in 3a094f0

[key: string]: any;

maybe just use that like this?

const serverOptions: http2.ServerOptions = {
      maxSendHeaderBlockLength: Number.MAX_SAFE_INTEGER,
      ...this._setupHttp2SessionOptions()
};

....

private _setupHttp2SessionOptions(): http2.SessionOptions {
    const options: http2.SessionOptions = {};
    if ('http2.max_session_memory' in this.options) {
        options.maxSessionMemory = this.options['http2.max_session_memory'];
    }
    return options;
}
....

By the way, the problem is not only that it is impossible to send data larger than 10 megabytes, but also that under load with a large number of connections through which a lot of data is transmitted, this data queued for send which leads to frequent disconnections by session because default limit is exceeded very often in this case

[murgatroid99]

It looks like you're suggesting adding the option using the http2 prefix, and also not declaring it in the API for some reason. That prefix might be OK, but we absolutely want to have all options available to the user declared in the API.

[mnasyrov]

@dwrip @murgatroid99 Hi! Could you please update the status of this PR? I have the same issue with the default maxSessionMemory option in production, and I'm looking forward for this PR.

[mnasyrov]

@murgatroid99 @dwrip @jasonpraful Guys, I can take over this PR if you have no free time. Could you please list things which must be done to complete the PR?

[dwrip]

@mnasyrov I apologize for not answering for a long time. A lot of work fell on me. I will try to update the pr as soon as possible in accordance with the comments from @murgatroid99

[murgatroid99]

I have also had other things to work on. I have been giving this some thought, and I think the right approach is to use the option name grpc-node.max_session_memory. This keeps the naming scheme broadly the same, but with a namespace specific to this library.

[dwrip]

@murgatroid99 Hi, I updated the pr

[murgatroid99]

Thank you for your contribution.

[murgatroid99]

This change has been published in grpc-js 1.3.0.

[NeoyeElf]

I found the doc is outdated, the grpc-node.max_session_memory options is missing.