Add CMux.Close() to shutdown server #69
Conversation
|
@jan25 |
|
@rrylee Let me fix the CI and do few tests when i get a chance. Also i don't have Merge rights, so owners of this repo suggestions on this PR? @soheilhy |
|
@jan25 is there a chance this can be merged? |
|
Hi @Krithika3. I can only try to finish up this PR. I will need maintainers help to review and merge as i don't have the rights. Looks like the last release was more than 2yrs ago, so not sure if this will ever be merged. |
Signed-off-by: Abhilash Gnan <[email protected]>
Signed-off-by: Abhilash Gnan <[email protected]>
|
@soheilhy Any way to get this merged? https://github.com/grpc/grpc-go expects calls to |
|
Hey sorry for the delay. @jan25 can you please add a test for this? otherwise, looks ok. |
This PR uses the cmux package to expose a single external port that selects incoming connections to route to the gRPC endpoint or the REST endpoint. (ref: https://medium.com/@drgarcia1986/listen-grpc-and-http-requests-on-the-same-port-263c40cb45ff and https://github.com/soheilhy/cmux) I define an Endpoint interface wrapping both the gRPC and the REST endpoints, as well as the cmux multiplexer that does the redirection. This infrastructure will also allow future endpoints to be easily added when we need them. This also sets up the infrastructure for graceful shutdown upon receipt of a signal, though we are waiting for a PR in the cmux dependency for that to work fully. (The issue is described in soheilhy/cmux#69 (comment).) Implementation/review notes: - the gRPC server set-up code is now in `cmd/gapic-showcase/endpoint.go`. It is split between setting up the server and running it. - the REST server (also in `endpoint.go`) is currently a stub that simply responds to `/hello` requests. It will be expanded with auto-generated handlers.
|
@soheilhy thanks. I'll finalize this PR soon |
|
@jan25 This PR will be very useful! Do you have an ETA for getting it finished? Thanks for your work on this! |
Signed-off-by: Abhilash Gnan <[email protected]>
|
Sorry for late response. I have to fix/add tests, i'll spend some time this week to finish. |
Signed-off-by: Abhilash Gnan <[email protected]>
jan25
changed the title
|
I've added tests and fixed other breaking tests. @soheilhy could you please review? |
|
Looks great. Thank you for adding the tests! |
| @@ -93,6 +97,8 @@ type CMux interface { | |||
| // Serve starts multiplexing the listener. Serve blocks and perhaps | |||
| // should be invoked concurrently within a go routine. | |||
| Serve() error | |||
| // Closes cmux server and stops accepting any connections on listener | |||
| Close() | |||
There was a problem hiding this comment.
error can inform user about multiple Close() calls. Are there other cases when error can be useful?
There was a problem hiding this comment.
The principle interest is in satisfying the interface { Close() error } (i.e. the io.Closer interface) more than anything else.
For instance, net.Listener for instance implements io.Closer.
But it’s definitely not anything critical, just expected that Close() returns an error.
| sls []matchersListener | ||
| readTimeout time.Duration | ||
| donec chan struct{} |
There was a problem hiding this comment.
Try to put things that are protected by a mutex underneath the mutex.
|
So glad to see this great prgress, do we have a plan to draft a new release? |
We don't have a nice way to close CMux server. This PR adds a Close method that tries to gracefully stop cmux server.
Related issue: #39