[client] Revert migrate deprecated grpc client code#4805
Conversation
WalkthroughThe gRPC client dialer is refactored to simplify connection initialization by replacing Changes
Sequence DiagramsequenceDiagram
participant Caller
participant Dialer
participant gRPC as gRPC Lib
rect rgb(200, 220, 255)
Note over Caller,gRPC: Old Flow (Removed)
Caller->>Dialer: Create connection
Dialer->>gRPC: NewClient()
gRPC-->>Dialer: conn (possibly not ready)
Dialer->>Dialer: waitForConnectionReady()
Dialer->>gRPC: Poll connection state
gRPC-->>Dialer: Ready
Dialer-->>Caller: conn
end
rect rgb(220, 240, 220)
Note over Caller,gRPC: New Flow (Simplified)
Caller->>Dialer: Create connection
Dialer->>Dialer: Create 30s timeout context
Dialer->>gRPC: DialContext(ctx, WithBlock())
gRPC-->>Dialer: conn (blocks until ready)
Dialer-->>Caller: conn
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
shared/management/client/grpc.go (1)
58-60: Consider preserving error context through wrapping.The change removes error wrapping in favor of logging, which loses contextual information about where the error occurred. While this simplifies the error handling, it makes debugging connection issues more difficult as the error no longer indicates it came from
CreateConnection.Consider maintaining error context while still logging:
var err error conn, err = nbgrpc.CreateConnection(ctx, addr, tlsEnabled, wsproxy.ManagementComponent) if err != nil { - log.Printf("createConnection error: %v", err) - return err + log.Printf("createConnection error: %v", err) + return fmt.Errorf("create connection: %w", err) } return nilshared/signal/client/grpc.go (1)
63-65: Consider preserving error context through wrapping.Similar to the management client, this change removes error wrapping which loses contextual information about where the error occurred. This makes debugging signal connection issues more difficult.
Consider maintaining error context while still logging:
var err error conn, err = nbgrpc.CreateConnection(ctx, addr, tlsEnabled, wsproxy.SignalComponent) if err != nil { - log.Printf("createConnection error: %v", err) - return err + log.Printf("createConnection error: %v", err) + return fmt.Errorf("create connection: %w", err) } return nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
client/grpc/dialer.go(1 hunks)client/grpc/dialer_generic.go(2 hunks)client/net/conn.go(2 hunks)client/net/dial.go(1 hunks)client/net/dialer_dial.go(1 hunks)client/net/listener_listen.go(2 hunks)shared/management/client/grpc.go(1 hunks)shared/signal/client/grpc.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
client/grpc/dialer_generic.go (1)
client/grpc/dialer_js.go (1)
WithCustomDialer(11-13)
client/grpc/dialer.go (2)
client/grpc/dialer_generic.go (1)
WithCustomDialer(21-44)client/grpc/dialer_js.go (1)
WithCustomDialer(11-13)
client/net/conn.go (1)
client/net/hooks/hooks.go (2)
ConnectionID(13-13)GetCloseHooks(82-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Signal / Unit (386)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Signal / Unit (amd64)
- GitHub Check: Client / Unit (386)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: Linux
- GitHub Check: Windows
- GitHub Check: Darwin
- GitHub Check: release_ui_darwin
- GitHub Check: release_ui
- GitHub Check: release
- GitHub Check: iOS / Build
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
🔇 Additional comments (4)
client/net/dial.go (1)
77-77: LGTM: Formatting change.This blank line improves code readability with no functional impact.
client/net/dialer_dial.go (1)
67-67: LGTM: Error message clarification.The addition of "failed to" improves error message clarity.
client/grpc/dialer.go (1)
45-62: Confirm the justification for reverting the grpc.NewClient migration.The review comment's concerns are factually valid: the code deliberately uses the deprecated
grpc.DialContextAPI with blocking behavior viagrpc.WithBlock(). Git history confirms this is commit6ae17c8which reverts commit43c9a51("[client] Migrate deprecated grpc client code")—this is an intentional reversion.The deprecated API concerns remain valid:
- DialContext is the older, deprecated API; NewClient is recommended
WithBlock()enforces synchronous connection establishment, changing from lazy to blocking semantics- DialContext uses legacy "passthrough" resolver while NewClient uses "dns" resolver
However, since this reversion is recent and deliberate, the key missing information is:
- Why was the migration to
grpc.NewClient(PR #4687) reverted instead of fixing any issues it introduced?- What problems did the migration cause that required reverting?
Verify and document the rationale for this intentional reversion before merging.
client/grpc/dialer_generic.go (1)
21-43: No action required—parameters are intentionally unused due to platform-specific implementation.The function uses Go's build tags (
//go:build !js) for conditional compilation. Thedialer_generic.goimplementation is compiled for non-JS platforms where the parameters are unused, whiledialer_js.gouses both parameters for WebSocket proxy configuration. The identical function signature across both implementations ensures caller compatibility. This is intentional and correct.
| // Close overrides the net.Conn Close method to execute all registered hooks after closing the connection | ||
| // Close overrides the net.Conn Close method to execute all registered hooks before closing the connection. |
There was a problem hiding this comment.
Critical: Documentation contradicts implementation.
The comments on lines 21 and 32 state that hooks execute "before closing the connection," but the closeConn implementation on lines 38-49 clearly shows that conn.Close() is called first (line 39), and then the hooks are executed (lines 41-46). The documentation is incorrect and misleading.
Apply this diff to correct the documentation:
-// Close overrides the net.Conn Close method to execute all registered hooks before closing the connection.
+// Close overrides the net.Conn Close method to execute all registered hooks after closing the connection.
func (c *Conn) Close() error {
return closeConn(c.ID, c.Conn)
}
// TCPConn wraps net.TCPConn to override its Close method to include hook functionality.
type TCPConn struct {
*net.TCPConn
ID hooks.ConnectionID
}
-// Close overrides the net.TCPConn Close method to execute all registered hooks before closing the connection.
+// Close overrides the net.TCPConn Close method to execute all registered hooks after closing the connection.
func (c *TCPConn) Close() error {
return closeConn(c.ID, c.TCPConn)
}Also applies to: 32-32, 38-49
🤖 Prompt for AI Agents
In client/net/conn.go around lines 20-21, line 32 and lines 38-49, the comments
incorrectly state that registered hooks run "before closing the connection"
while the implementation calls conn.Close() first and then runs the hooks;
update the doc comments at those locations to say that hooks are executed after
the connection is closed (and make the wording consistent across all affected
comment lines), e.g., change "before closing" to "after closing" and adjust the
sentence to match the implementation behavior.
This reverts commit 43c9a51.
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/grpc/dialer.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
client/grpc/dialer.go (2)
client/grpc/dialer_generic.go (1)
WithCustomDialer(21-43)client/grpc/dialer_js.go (1)
WithCustomDialer(11-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Signal / Unit (amd64)
- GitHub Check: Client / Unit (386)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Windows
- GitHub Check: Darwin
- GitHub Check: Linux
- GitHub Check: iOS / Build
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
- GitHub Check: release_ui_darwin
- GitHub Check: release
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
🔇 Additional comments (1)
client/grpc/dialer.go (1)
29-65: The review comment is incorrect.The exported symbols
ErrConnectionShutdownandwaitForConnectionReadydo not exist in the grpc package and are not referenced anywhere in the codebase. The grpc package only exportsBackoff,CreateConnection, andWithCustomDialer. No breaking API changes were introduced in this PR.Likely an incorrect or invalid review comment.
| connCtx, cancel := context.WithTimeout(ctx, 30*time.Second) | ||
| defer cancel() | ||
|
|
||
| conn, err := grpc.DialContext( | ||
| connCtx, | ||
| addr, | ||
| transportOption, | ||
| WithCustomDialer(tlsEnabled, component), | ||
| grpc.WithBlock(), | ||
| grpc.WithKeepaliveParams(keepalive.ClientParameters{ | ||
| Time: 30 * time.Second, | ||
| Timeout: 10 * time.Second, | ||
| }), | ||
| ) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("new client: %w", err) | ||
| } | ||
|
|
||
| ctx, cancel := context.WithTimeout(ctx, 30*time.Second) | ||
| defer cancel() | ||
|
|
||
| if err := waitForConnectionReady(ctx, conn); err != nil { | ||
| _ = conn.Close() | ||
| return nil, err | ||
| return nil, fmt.Errorf("dial context: %w", err) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify: This reverts to deprecated gRPC APIs that should not be used with gRPC v1.73.0.
The code now uses grpc.DialContext and grpc.WithBlock(), both of which were deprecated in gRPC v1.70.0. Since the project uses gRPC v1.73.0, this revert introduces deprecated API usage. The modern approach is grpc.NewClient with explicit connection state management if needed.
Please verify the deprecation status and recommended migration path for these gRPC APIs in version 1.73.0:
Additionally, check if there was a specific bug or issue with the original grpc.NewClient implementation that necessitated this revert:
🏁 Script executed:
#!/bin/bash
# Search for related issues, bug reports, or comments about grpc.NewClient problems
rg -n -C5 'grpc\.NewClient|waitForConnectionReady|ErrConnectionShutdown' --type go
# Check git history for the original migration commit and this revert
git log --oneline --grep="grpc" --all | head -20
# Look for any TODO/FIXME/BUG comments related to gRPC connection handling
rg -n 'TODO|FIXME|BUG|HACK' --type go -g '*grpc*' -C3Length of output: 3387
🌐 Web query:
grpc.DialContext grpc.WithBlock deprecated gRPC 1.73.0 alternative
💡 Result:
Short answer: stop using grpc.DialContext(..., grpc.WithBlock()) — use grpc.NewClient (introduced v1.63) and, if you must block until the channel is ready, call the ClientConn's Connect + WaitForStateChange / check connectivity state yourself. grpc.NewClient does no I/O by default; Dial/DialContext and the WithBlock/WithTimeout DialOptions are deprecated. [1][2][3]
Example sketch:
- cc := grpc.NewClient(target, opts...) // no I/O
- if err := cc.Connect(ctx); err != nil { ... } // triggers connection attempt
- // optionally wait for Ready via cc.WaitForStateChange / cc.GetState loop
References:
- grpc-go package docs (NewClient, DialContext deprecation). [1]
- grpc-go anti-patterns / guidance recommending NewClient. [2]
- clientconn implementation notes describing DialContext behavior (WithBlock behaviour). [3]
This revert to deprecated gRPC APIs contradicts gRPC v1.73.0 best practices and creates codebase inconsistency.
The grpc.DialContext and grpc.WithBlock() APIs are deprecated in gRPC v1.70.0 and later in favor of grpc.NewClient. Since the project uses gRPC v1.73.0, this change reintroduces deprecated APIs that should not be used.
Additionally, other files in the codebase (flow/client/client.go:68, client/ui/event/event.go:167) continue using the modern grpc.NewClient approach, creating inconsistency.
Required changes:
- Revert to
grpc.NewClient(modern API) - If blocking connection state is required, use
ClientConn.Connect(ctx)instead ofgrpc.WithBlock()
2 participants
This reverts commit 43c9a51.
Describe your changes
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit