Skip to content

fix: script leak and shut down and remove per-message goroutine#69

Merged
swarna1101 merged 2 commits intomainfrom
singh/minor-fixe
Feb 5, 2026
Merged

fix: script leak and shut down and remove per-message goroutine#69
swarna1101 merged 2 commits intomainfrom
singh/minor-fixe

Conversation

@hpsing
Copy link
Member

@hpsing hpsing commented Feb 4, 2026

  • safe drain
  • remove per message goroutine (fan-out in subscribe)
  • fix error/cancel in publish/subsribe worker

Summary by CodeRabbit

  • Bug Fixes

    • Corrected several command-line error messages for clearer feedback.
  • Improvements

    • Added stricter input validation with clearer failures for publish/subscribe options.
    • Improved error aggregation so worker failures stop the run and surface consistently.
    • Enhanced file-output behavior to ensure orderly flushing and a final "all data flushed" notice.
    • Simplified and made message reception more reliable.
  • Documentation

    • Updated usage examples and Docker Compose port guidance for multi-node runs; added notes on index ranges and a new data-size option.

@coderabbitai
Copy link

coderabbitai bot commented Feb 4, 2026

📝 Walkthrough

Walkthrough

Refactors CLI clients to centralize per-worker error aggregation and cancellation, simplifies subscribe receive loops, validates input flags, and updates file-writing to a cancellable writer with header and filename parameters. No public API additions.

Changes

Cohort / File(s) Summary
Multi-publish
grpc_p2p_client/cmd/multi-publish/main.go
Added input validation, derived randomByteLen, per-worker error channel (errCh) and context cancellation on worker error; moved header/writer init to top-level; changed sendMessages to return nil on ctx cancel and localize data buffer.
Multi-subscribe
grpc_p2p_client/cmd/multi-subscribe/main.go
Fixed flag help text, added errCh to aggregate worker errors and cancel on failure; receiveMessages now returns an error and reads stream.Recv() directly; removed internal msg channel/goroutine and simplified logging and shutdown sequence.
Single command
grpc_p2p_client/cmd/single/main.go
Replaced goroutine+channel+select receive pattern with direct stream.Recv() loop and inline HandleResponse; fixed flag help text formatting.
Shared utilities
grpc_p2p_client/shared/utils.go
ReadIPsFromFile validates non-empty filename; WriteToFile signature changed to accept filename and header, is context-aware (select over ctx.Done/dataCh), drains remaining data on shutdown, and signals completion via done.
Docs
docs/guide.md, readme.md
Documentation and examples updated for multi-node Docker port mappings, new flags (-datasize), and index-range usage in examples.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant Main
participant Worker
participant ErrCollector
participant FileWriter
participant Context
Main->>Worker: spawn worker (publish/subscribe)
Worker->>ErrCollector: send error (if any)
ErrCollector->>Main: notify error
Main->>Context: cancel()
Main->>FileWriter: close / stop writer (signal done)
FileWriter-->>Main: flush complete / done

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • paiva

Poem

🐇 I hopped through streams and channel lanes,

Collected errors, calmed the strains,
Writers hush when contexts cry,
Messages now pass in a simpler sigh,
Code hops light beneath spring rains.

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly addresses the main changes: removing per-message goroutines and fixing error/cancellation handling in publish/subscribe workers, which aligns with the changeset's core refactoring efforts.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch singh/minor-fixe

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@grpc_p2p_client/shared/utils.go`:
- Around line 246-250: Move the error check so you verify the result of
writer.WriteString(data + "\n") before calling writer.Flush(), and also handle
the error returned by writer.Flush(); specifically, after calling
writer.WriteString in the current function (where writer.WriteString and
writer.Flush are used) check if err != nil and log/return immediately, then call
writer.Flush() and check its error too (log/return if non-nil). Reference the
existing symbols writer.WriteString and writer.Flush to locate the code to
change.
🧹 Nitpick comments (1)
grpc_p2p_client/shared/utils.go (1)

247-247: Consider batching flushes for better performance.

Calling Flush() after every write negates the benefits of buffered I/O. For high-throughput scenarios, this could be a performance bottleneck. Consider flushing periodically (e.g., every N writes or on a timer) or relying on the deferred Flush() at function exit.

Copy link
Collaborator

@swarna1101 swarna1101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@swarna1101 swarna1101 merged commit bd8c0b8 into main Feb 5, 2026
6 checks passed
@swarna1101 swarna1101 deleted the singh/minor-fixe branch February 5, 2026 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants