Return server.ErrPeerNotConnected error #11
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR standardizes error handling by returning a predefined server.ErrPeerNotConnected error instead of creating custom gRPC status errors when peer connections are unavailable.
- Updates dependencies, particularly the netbird library and OpenTelemetry packages
- Replaces custom gRPC status errors with a standardized
server.ErrPeerNotConnectederror for consistent error handling
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| dispatcher/go.mod | Updates dependencies including netbird library and OpenTelemetry packages |
| dispatcher/dispatcher.go | Replaces custom gRPC status errors with standardized server.ErrPeerNotConnected error |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
dispatcher/dispatcher.go
Outdated
| if !ok { | ||
| log.Tracef("message from peer [%s] can't be forwarded to peer [%s] because destination peer is not connected", msg.Key, msg.RemoteKey) | ||
| return &proto.EncryptedMessage{}, status.Errorf(codes.NotFound, "remote peer not connected") | ||
| return &proto.EncryptedMessage{}, server.ErrPeerNotConnected |
There was a problem hiding this comment.
The function signature indicates it should return a gRPC status error, but server.ErrPeerNotConnected may not be a proper gRPC status error. This could break gRPC error handling conventions and cause issues for clients expecting standard gRPC status codes.
Copilot uses AI. Check for mistakes.
dispatcher/dispatcher.go
Outdated
| // Channel was disconnected after we took it from the map | ||
| log.Tracef("message from peer [%s] can't be forwarded to peer [%s] because destination peer is not connected", msg.Key, msg.RemoteKey) | ||
| return &proto.EncryptedMessage{}, status.Errorf(codes.Aborted, "remote peer disconnected") | ||
| return &proto.EncryptedMessage{}, server.ErrPeerNotConnected |
There was a problem hiding this comment.
Similar to the previous case, returning server.ErrPeerNotConnected instead of a gRPC status error may break error handling conventions. The original codes.Aborted provided specific context about the disconnection timing that is now lost.
| return &proto.EncryptedMessage{}, server.ErrPeerNotConnected | |
| return nil, status.Errorf(codes.Aborted, "destination peer [%s] is not connected (disconnected after channel lookup)", msg.RemoteKey) |
Copilot uses AI. Check for mistakes.
2 participants
No description provided.