fix: prevent libp2p panic by reusing gRPC connections in multi-publish#68
Conversation
📝 WalkthroughWalkthroughThe PR updates Go toolchain versions across the codebase from 1.24.1 to 1.25.5, bumps CI linting tool version, refactors GRPC connection handling in the multi-publish client to reuse connections across iterations instead of recreating per-iteration, adds log file ignores, and cleans up module dependencies. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
grpc_p2p_client/cmd/multi-publish/main.go (1)
79-84: Error return value is ignored, silently discarding connection and stream failures.
sendMessagesreturns an error, but the caller discards it. This means connection failures, stream errors, and context cancellations go unreported—undermining the PR's goal of fixing connection issues.🔧 Proposed fix to handle the error
go func(ip string) { defer wg.Done() datasize := *dataSize - sendMessages(ctx, ip, datasize, *output != "", dataCh) + if err := sendMessages(ctx, ip, datasize, *output != "", dataCh); err != nil { + log.Printf("Error sending messages to %s: %v", ip, err) + } }(ip)
🧹 Nitpick comments (3)
grpc_p2p_client/cmd/multi-publish/main.go (3)
106-110: Stream is never closed; consider callingCloseSend()after the loop.For bidirectional gRPC streams, the client should call
stream.CloseSend()when finished sending to properly signal the server. Without this, the server may not know the client is done, potentially causing resource leaks or hangs.♻️ Proposed fix to close the stream
client := protobuf.NewCommandStreamClient(conn) stream, err := client.ListenCommands(ctx) if err != nil { return fmt.Errorf("[%s] ListenCommands failed: %w", ip, err) } + defer stream.CloseSend() println(fmt.Sprintf("Connected to node at: %s…", ip))
112-112: Usefmt.Printforlog.Printfinstead ofprintln.
printlnis a built-in intended for bootstrapping/debugging, not production code. It writes to stderr and has inconsistent formatting across platforms.♻️ Proposed fix
- println(fmt.Sprintf("Connected to node at: %s…", ip)) + log.Printf("Connected to node at: %s…", ip)
71-74: Simplify nested goroutine by removing unnecessary wrapper.The outer goroutine only spawns an inner goroutine and exits immediately. This adds complexity without benefit.
♻️ Proposed simplification
if *output != "" { done = make(chan bool) - go func() { - header := "sender\tsize\tsha256(msg)" - go shared.WriteToFile(ctx, dataCh, done, *output, header) - }() + header := "sender\tsize\tsha256(msg)" + go shared.WriteToFile(ctx, dataCh, done, *output, header) }
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.