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

@grpc/grpc-js & SO_REUSEPORT #1547

Closed
hugebdu opened this issue Aug 19, 2020 · 10 comments
Closed

@grpc/grpc-js & SO_REUSEPORT #1547

hugebdu opened this issue Aug 19, 2020 · 10 comments

Comments

@hugebdu
Copy link
Contributor

hugebdu commented Aug 19, 2020

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

Another discrepancy with grpc-native we found on linux is inability to bind multiple grpc.Server instances on the same port in @grpc/grpc-js.

We have a requirement to load/unload services without downtime and since both implementations do not allow that what we did was bringing up a new instance of server with the new set of services and gracefully shutting down the previous instance. Worked like magic with grpc-native.

Didn't dive too deep but it looks like the problem goes all the way to node's http2 and even nghttp2. At least I couldn't find any mentions of that option.

@hugebdu
Copy link
Contributor Author

hugebdu commented Aug 19, 2020

UPD
apparently it looks like this limitation is here for historical reasons. I just removed those lines and successfully mutated handlers map (via handlers.clear() and addService(...)) without restart.

have all my contract tests passing.

To sum up - any chance you will consider runtime mutation APIs or should I stay with my monkey patching on my own risk?

@murgatroid99
Copy link
Member

I'm sorry, I meant to reply to this earlier. As far as I can tell, there was never a strong reason to have that restriction on registering services after starting the server, especially on Node, where there isn't any significant concern about synchronizing between mutating the registered handlers map and dispatching incoming requests. So, I would be OK with removing that restriction entirely.

Removing services is a little trickier because there isn't an existing API for that, so we'll have to add one. The handlers member is private, so modifying it directly isn't the right solution.

Also, regarding the original question about SO_REUSEPORT, Node does not provide any JavaScript API to set that option. That's less about nghttp2, and more about Node's net built in module.

@hugebdu
Copy link
Contributor Author

hugebdu commented Sep 3, 2020

Hey Michael.

I guess there could be Server#unregister(name: string, ....) - the counterpart of Server#register(...).

On top of that we can have Server#removeService(...) that accepts either instance of ServiceDefinition or some handle returned by addService (requires changing the return type of addService).

WDYT?

@murgatroid99
Copy link
Member

I think it can just be Server#unregister(name:string) and Server#removeService(service: ServiceDefinition). You can't register the same name/path more than once, so the name uniquely identifies a registered handler, and a service uniquely identifies a list of registered paths.

@hugebdu
Copy link
Contributor Author

hugebdu commented Sep 6, 2020

Sounds great. I can try and make a PR. Just need to make sure I understand all the tests setup.
Is it enough to add new tests only to the @grpc-js or you expect also new tests being added to the cross implementation tests?

@murgatroid99
Copy link
Member

I'm sorry for the silence here. I thought I responded to the last comment. If you're still interested, tests for this would just need to go in the grpc-js test directory because we would not want to replicate this change for the other implementation.

@hugebdu
Copy link
Contributor Author

hugebdu commented Oct 29, 2020

@murgatroid99 here's my PR #1614

linter checks fail in posttest script, however it fails on master as well. probably the fix script glob should include subfolders?

@hugebdu
Copy link
Contributor Author

hugebdu commented Oct 29, 2020

@murgatroid99 thanks!
IMO this one can be closed

@zyf0330
Copy link

zyf0330 commented Nov 12, 2020

Hello, when does this feature SO_REUSEPORT release in @grpc/grpc-js?

@murgatroid99
Copy link
Member

The title doesn't exactly match what this issue ended up being about. It is primarily a request for the ability to add and remove services from a running server, and that is implemented in the latest release 1.2.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants