-
Notifications
You must be signed in to change notification settings - Fork 640
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
feature(grpc-js): Add possibility to provide maxSessionMemory http2 option through ChannelOptions #1666
Conversation
…ption through ChannelOptions
|
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 |
What specific error details did you get with the |
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. |
For example, if the server responds with a data ( 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
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. |
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. |
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
|
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. |
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. :) |
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. |
Ok. I understand. in channel options interface you have additional field
maybe just use that like this?
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 |
It looks like you're suggesting adding the option using the |
@dwrip @murgatroid99 Hi! Could you please update the status of this PR? I have the same issue with the default |
@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? |
@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 |
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 |
@murgatroid99 Hi, I updated the pr |
Thank you for your contribution. |
This change has been published in grpc-js 1.3.0. |
…r read in the code). Therefore, declaring those ChannelOptions had no effect anymore and there was no way to configure specific gRPC settings & limits. Also extended ChannelOptions with grpc-node.max_session_memory which is required when sending big messages (see grpc/grpc-node#1666). Ensured backward compatibility by including size limits like grpc.max_send_message_length to the ChannelOptions
I found the doc is outdated, the |
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.