feat: add some grpc utils #403
Conversation
WalkthroughThis set of changes introduces new abstractions and refactors existing code to unify and simplify server lifecycle management for both HTTP and gRPC servers. A new Changes
Sequence Diagram(s)sequenceDiagram
participant FxApp
participant ServerPort
participant gRPCServer
participant Context
FxApp->>ServerPort: NewServer(discr, opts...)
FxApp->>ServerPort: Listen(ctx)
ServerPort->>Context: ContextWithServerInfo(ctx, discr)
FxApp->>gRPCServer: New(grpcOpts...)
FxApp->>gRPCServer: Apply setup options
FxApp->>gRPCServer: Serve(ServerPort.Listener)
Note over gRPCServer: Server runs asynchronously
FxApp->>gRPCServer: Stop() on shutdown
sequenceDiagram
participant FxApp
participant ServerPort
participant HTTPServer
participant Context
FxApp->>ServerPort: NewServer("http", opts...)
FxApp->>ServerPort: Listen(ctx)
ServerPort->>Context: ContextWithServerInfo(ctx, "http")
FxApp->>HTTPServer: New(handler, httpOpts...)
FxApp->>HTTPServer: Serve(ServerPort.Listener)
Note over HTTPServer: Server runs asynchronously
FxApp->>HTTPServer: Shutdown() on stop
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #403 +/- ##
==========================================
- Coverage 35.11% 34.29% -0.83%
==========================================
Files 124 127 +3
Lines 5263 5389 +126
==========================================
Hits 1848 1848
- Misses 3306 3432 +126
Partials 109 109 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
serverport/serverport.go (2)
14-22: Potential panic risk with type assertion.The type assertion on line 21 (
siAsAny.(*ServerInfo)) lacks a safety check, which could cause a panic if the wrong type is stored in the context. Although this should not happen if only this package manipulates the context value, it's best to use a safer approach.Consider modifying the code to use the safer two-value type assertion pattern:
func GetActualServerInfo(ctx context.Context, discr string) *ServerInfo { siAsAny := ctx.Value(serverInfoContextKey(discr)) if siAsAny == nil { return nil } - return siAsAny.(*ServerInfo) + si, ok := siAsAny.(*ServerInfo) + if !ok { + return nil + } + return si }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 16-21: serverport/serverport.go#L16-L21
Added lines #L16 - L21 were not covered by tests
57-61: Consider adding thread safety mechanisms.The
Serverstruct holds state that might be accessed concurrently, especially when used in a server context. Without proper synchronization, this could lead to race conditions.Consider adding a mutex to protect access to the fields, especially when updating the listener:
type Server struct { Listener net.Listener Address string Discr string + mu sync.Mutex }And then protect critical sections with lock/unlock operations.
testing/testservice/instrumentation_grpcserver.go (1)
18-20: Consider adding error handling.The
GetGRPCAddressfunction doesn't handle the case where the address might not be available yet. If called before the server is fully initialized, it will simply return an empty string with no indication of the failure condition.Consider returning an error or providing a timeout-based approach:
-func GetGRPCAddress(service *Service) string { +func GetGRPCAddress(service *Service) (string, error) { addr := grpcserver.Address(service.GetContext()) + if addr == "" { + return "", errors.New("gRPC server address not available") + } - return grpcserver.Address(service.GetContext()) + return addr, nil }grpcserver/serverport.go (1)
34-38: Graceful shutdown should also close the listener
grpc.Server.GracefulStop()stops accepting new connections but does not close the underlying listener.
When a custom listener is injected throughserverport.WithListener, the application may hold on to the socket after shutdown.return func(ctx context.Context) error { grpcServer.GracefulStop() + if s.Listener != nil { + _ = s.Listener.Close() + } return nil }, nilThis prevents “address already in use” errors on subsequent restarts (handy in tests).
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 34-38: grpcserver/serverport.go#L34-L38
Added lines #L34 - L38 were not covered by testshttpserver/serverport.go (1)
42-44: Avoid panicking on serve errors – use structured logging like the gRPC pathA panic will tear down the entire Fx app, whereas a logged error (plus returning the error through a channel if you want to fail the lifecycle) gives callers a chance to react.
- err := srv.Serve(s.Listener) - if err != nil && err != http.ErrServerClosed { - panic(err) + if err := srv.Serve(s.Listener); err != nil && !errors.Is(err, http.ErrServerClosed) { + logging.FromContext(ctx).Errorf("HTTP server failed to serve: %v", err) + }This change keeps behaviour consistent with the gRPC starter and avoids unexpected crashes in production.
(Requireserrorsimport.)🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 42-42: httpserver/serverport.go#L42
Added line #L42 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.modis excluded by!**/*.mod
📒 Files selected for processing (6)
grpcserver/serverport.go(1 hunks)httpserver/serverport.go(4 hunks)serverport/serverport.go(1 hunks)testing/deferred/deferred.go(2 hunks)testing/platform/clickhousetesting/clickhouse.go(4 hunks)testing/testservice/instrumentation_grpcserver.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
testing/testservice/instrumentation_grpcserver.go (4)
testing/testservice/instrumentation.go (3)
Instrumentation(35-37)InstrumentationFunc(38-38)RunConfiguration(8-13)serverport/serverport.go (2)
ContextWithServerInfo(24-28)Address(38-44)grpcserver/serverport.go (2)
ContextWithServerInfo(14-16)Address(41-43)testing/testservice/service.go (1)
Service(15-22)
httpserver/serverport.go (2)
serverport/serverport.go (7)
ContextWithServerInfo(24-28)Address(38-44)Server(57-61)ServerOpts(92-92)WithListener(94-98)WithAddress(100-104)NewServer(80-90)grpcserver/serverport.go (6)
ContextWithServerInfo(14-16)Address(41-43)ServerOptions(45-49)WithServerPortOptions(53-57)ServerOptionModifier(51-51)NewHook(71-106)
grpcserver/serverport.go (3)
serverport/serverport.go (5)
ContextWithServerInfo(24-28)Server(57-61)NewServer(80-90)Address(38-44)ServerOpts(92-92)httpserver/serverport.go (5)
ContextWithServerInfo(17-19)ServerOptions(53-56)WithServerPortOptions(60-64)ServerOptionModifier(58-58)NewHook(80-116)logging/context.go (1)
FromContext(12-20)
🪛 GitHub Check: codecov/patch
serverport/serverport.go
[warning] 16-21: serverport/serverport.go#L16-L21
Added lines #L16 - L21 were not covered by tests
[warning] 24-27: serverport/serverport.go#L24-L27
Added lines #L24 - L27 were not covered by tests
[warning] 30-35: serverport/serverport.go#L30-L35
Added lines #L30 - L35 were not covered by tests
[warning] 38-43: serverport/serverport.go#L38-L43
Added lines #L38 - L43 were not covered by tests
[warning] 46-50: serverport/serverport.go#L46-L50
Added lines #L46 - L50 were not covered by tests
[warning] 52-54: serverport/serverport.go#L52-L54
Added lines #L52 - L54 were not covered by tests
[warning] 63-72: serverport/serverport.go#L63-L72
Added lines #L63 - L72 were not covered by tests
httpserver/serverport.go
[warning] 18-18: httpserver/serverport.go#L18
Added line #L18 was not covered by tests
[warning] 24-24: httpserver/serverport.go#L24
Added line #L24 was not covered by tests
[warning] 27-27: httpserver/serverport.go#L27
Added line #L27 was not covered by tests
[warning] 29-30: httpserver/serverport.go#L29-L30
Added lines #L29 - L30 were not covered by tests
[warning] 42-42: httpserver/serverport.go#L42
Added line #L42 was not covered by tests
[warning] 60-62: httpserver/serverport.go#L60-L62
Added lines #L60 - L62 were not covered by tests
[warning] 66-67: httpserver/serverport.go#L66-L67
Added lines #L66 - L67 were not covered by tests
[warning] 70-71: httpserver/serverport.go#L70-L71
Added lines #L70 - L71 were not covered by tests
[warning] 74-75: httpserver/serverport.go#L74-L75
Added lines #L74 - L75 were not covered by tests
[warning] 80-80: httpserver/serverport.go#L80
Added line #L80 was not covered by tests
[warning] 86-89: httpserver/serverport.go#L86-L89
Added lines #L86 - L89 were not covered by tests
[warning] 91-93: httpserver/serverport.go#L91-L93
Added lines #L91 - L93 were not covered by tests
[warning] 102-102: httpserver/serverport.go#L102
Added line #L102 was not covered by tests
grpcserver/serverport.go
[warning] 14-15: grpcserver/serverport.go#L14-L15
Added lines #L14 - L15 were not covered by tests
[warning] 18-22: grpcserver/serverport.go#L18-L22
Added lines #L18 - L22 were not covered by tests
[warning] 24-31: grpcserver/serverport.go#L24-L31
Added lines #L24 - L31 were not covered by tests
[warning] 34-38: grpcserver/serverport.go#L34-L38
Added lines #L34 - L38 were not covered by tests
[warning] 41-42: grpcserver/serverport.go#L41-L42
Added lines #L41 - L42 were not covered by tests
[warning] 53-56: grpcserver/serverport.go#L53-L56
Added lines #L53 - L56 were not covered by tests
[warning] 59-62: grpcserver/serverport.go#L59-L62
Added lines #L59 - L62 were not covered by tests
[warning] 65-68: grpcserver/serverport.go#L65-L68
Added lines #L65 - L68 were not covered by tests
[warning] 71-80: grpcserver/serverport.go#L71-L80
Added lines #L71 - L80 were not covered by tests
[warning] 82-103: grpcserver/serverport.go#L82-L103
Added lines #L82 - L103 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
serverport/serverport.go (1)
80-90: Good use of functional options pattern.The implementation of
NewServerwith functional options is clean and follows Go best practices. This approach provides a flexible and extensible way to configure the server.testing/deferred/deferred.go (2)
53-53: Good fix: Clear error field on reset.Adding
d.err = nilensures that when a deferred operation is reset, any previous error state is properly cleared. This prevents stale errors from affecting future operations after a reset.
69-71: Improved error handling in Wait method.The change to check for errors before checking if the value is nil is a sound improvement. It properly prioritizes error reporting over nil value reporting, ensuring that errors are surfaced correctly to callers.
testing/testservice/instrumentation_grpcserver.go (1)
9-16: Clean implementation of gRPC server instrumentation.The
GRPCServerInstrumentationfunction effectively injects gRPC server information into the context. This is a clean approach that keeps server lifecycle management consistent with the patterns used elsewhere in the codebase.testing/platform/clickhousetesting/clickhouse.go (3)
59-75: Good resilience improvement with retry logic.The added retry logic for database cleanup is a solid improvement. It addresses potential race conditions when dropping databases by retrying the operation when ClickHouse responds with a "database not empty" error. The use of a reasonable max retry count (10) and delay (100ms) balances resilience with test responsiveness.
85-96: Well-implemented functional options pattern.The modification to
CreateServerto use functional options follows Go best practices and provides a clean, extensible way to configure the ClickHouse test server. The approach aligns with similar patterns used in the newserverportpackage.
131-145: Good addition of server configuration options.The implemented
ServerOptions,ServerOption, andWithVersioncomponents provide a flexible way to configure the ClickHouse server version. The default version (25.2-alpine) is sensibly chosen.
| func GetActualServerInfo(ctx context.Context, discr string) *ServerInfo { | ||
| siAsAny := ctx.Value(serverInfoContextKey(discr)) | ||
| if siAsAny == nil { | ||
| return nil | ||
| } | ||
| return siAsAny.(*ServerInfo) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Add unit tests for critical context manipulation functions.
The static analysis reports indicate that several key functions like GetActualServerInfo, ContextWithServerInfo, Started, Address, StartedServer, and Listen are not covered by tests. These functions are central to the package's functionality and should be thoroughly tested.
Also applies to: 24-28, 30-36, 38-44, 46-55, 63-78
🏁 Script executed:
#!/bin/bash
# Check if there are any test files for this package
fd -t f "_test.go" serverport/Length of output: 30
Let’s search more thoroughly for any Go test files or test functions in the serverport package:
🏁 Script executed:
#!/bin/bash
# Find any *_test.go files under serverport/
find serverport/ -type f -name "*_test.go"
# Search for Go test or benchmark functions in serverport/
rg -n "func (Test|Benchmark)[A-Za-z0-9_]*" serverport/Length of output: 101
Add comprehensive unit tests for serverport package
There are currently no _test.go files or test functions covering critical operations in serverport/serverport.go. Please add unit tests for:
- ContextWithServerInfo / GetActualServerInfo (lines 16–22)
- Started, Address
- StartedServer, Listen
These functions manipulate and extract server info via context.Context and form the core API; testing their behavior (valid key, missing key, nil handling, correct value retrieval) is essential.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 16-21: serverport/serverport.go#L16-L21
Added lines #L16 - L21 were not covered by tests
| go func() { | ||
| if err := grpcServer.Serve(s.Listener); err != nil { | ||
| logging.FromContext(ctx).Errorf("failed to serve: %v", err) | ||
| } | ||
| }() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Surface serving errors – align behaviour with the HTTP implementation
grpcServer.Serve can exit immediately (e.g. port already in use, TLS mis‑configuration).
In the HTTP starter we panic on such unrecoverable conditions, but here we only log, allowing the application to keep running in a half‑initialised state.
go func() {
- if err := grpcServer.Serve(s.Listener); err != nil {
- logging.FromContext(ctx).Errorf("failed to serve: %v", err)
+ if err := grpcServer.Serve(s.Listener); err != nil && !errors.Is(err, grpc.ErrServerStopped) {
+ panic(fmt.Errorf("GRPC server failed to serve: %w", err))
}
}()Doing so keeps both stacks consistent and fails fast when the port cannot be bound.
(Requires the errors and fmt imports.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| go func() { | |
| if err := grpcServer.Serve(s.Listener); err != nil { | |
| logging.FromContext(ctx).Errorf("failed to serve: %v", err) | |
| } | |
| }() | |
| go func() { | |
| if err := grpcServer.Serve(s.Listener); err != nil && !errors.Is(err, grpc.ErrServerStopped) { | |
| panic(fmt.Errorf("GRPC server failed to serve: %w", err)) | |
| } | |
| }() |
| server := serverport.NewServer(serverPortDiscr) | ||
| for _, option := range serverOptions.serverOptions { | ||
| option(server) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pass accumulated server‑port options directly when constructing the server
You already gather serverOptions.serverOptions; pass them to the constructor like the gRPC implementation and drop the follow‑up mutation loop for simplicity & immutability.
-server := serverport.NewServer(serverPortDiscr)
-for _, option := range serverOptions.serverOptions {
- option(server)
-}
+server := serverport.NewServer(serverPortDiscr, serverOptions.serverOptions...)Reduces cognitive load and the risk of future divergence between HTTP and gRPC helpers.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| server := serverport.NewServer(serverPortDiscr) | |
| for _, option := range serverOptions.serverOptions { | |
| option(server) | |
| } | |
| server := serverport.NewServer(serverPortDiscr, serverOptions.serverOptions...) |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 91-93: httpserver/serverport.go#L91-L93
Added lines #L91 - L93 were not covered by tests
| func WithHttpServerOpts(opts ...func(server *http.Server)) ServerOptionModifier { | ||
| return func(server *ServerOptions) { | ||
| server.httpServerOpts = opts | ||
| } |
There was a problem hiding this comment.
Functional‑option modifier overrides previous values instead of appending
WithHttpServerOpts replaces the slice each time it’s used, unlike the other modifiers that append.
Multiple call sites will silently lose options.
-return func(server *ServerOptions) {
- server.httpServerOpts = opts
+return func(server *ServerOptions) {
+ server.httpServerOpts = append(server.httpServerOpts, opts...)
}Unit tests configuring several timeouts/handlers would currently apply only the last one.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 74-75: httpserver/serverport.go#L74-L75
Added lines #L74 - L75 were not covered by tests
No description provided.