Handle UV server shutdown and allow socket activation #42
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit adds: - socket activation support, to allow the caller to pass a pre-existing bound tcp socket to server::run(). This is important in contexts where it is necessary to set some particular flags on the socket (such as keepalive), or if the socket is already created at program start (as is the case with the systemd socket activation protocol). - server shutdown support, to allow signaling the server via a std::stop_token that it should clean up and cleanly exit. Two unit tests were added. Running valgrind on the resulting binary also revealed a leak due to failure to cleanup a the uv_handle_t with the connection, this is now taken care of in the destructor of grpcxx::uv::server::loop_t. Valgrind does not report any more leaks during test execution.
uatuko
requested changes
Oct 30, 2024
There was a problem hiding this comment.
Great work as always 👏 and thanks for the PR!
There are a few things I'd tidy up with how I'd add tests but that's just personal preference and can be changed later (or not). I intentionally used g++ in macOS builds in the GitHub workflow since I normally use clang with macOS, I can check if I could figure out the issue with g++ when this is merged.
One request, I see you've added code comments which is great but could you update the API docs as well please?
This change adds the ability to subclass grpcxx::uv::server to be able to trigger individual loop iterations. This makes it possible to run the server as a nested loop inside either libuv, or optionally via a different framework (like libevent). Additionally, the following two bugs were fixed: - also support binding to IPv6 addresses - fix generation of source files from proto files when no package is specified. Note that, due to the current lack of a client-side gRPC implementation, the tests are not checking the full flow. Therefore, my approach was to add a sleep in the code and use Postman to check the service used in the tests answers correctly.
uatuko
reviewed
Oct 31, 2024
uatuko
approved these changes
Oct 31, 2024
uatuko
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change adds:
pre-existing bound tcp socket to server::run(). This
is important in contexts where it is necessary to set
some particular flags on the socket (such as keepalive),
or if the socket is already created at program start
(as is the case with the systemd socket activation protocol).
via a
std::stop_tokenthat it should clean up and cleanly exit.uv::serverand allowing iterating through the loop just once.
Three unit tests were added. Running valgrind on the resulting
binary also revealed a leak due to failure to cleanup a
the
uv_handle_twith the connection, this is now taken care ofin the destructor of
grpcxx::uv::server::loop_t. Valgrinddoes not report any more leaks during test execution.
Additionally, a bug in the generation of sources from proto
files when no package name is present was fixed.