Skip to content

fix: do not wrap Sender with Arc #6315

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
LesnyRumcajs merged 1 commit into from
Dec 8, 2025
Merged

fix: do not wrap Sender with Arc #6315

LesnyRumcajs merged 1 commit into from
Dec 8, 2025
+39 −15

Conversation

@hanabi1224
Copy link
Contributor

@hanabi1224 hanabi1224 commented Dec 8, 2025 •
edited by coderabbitai bot
Loading

Summary of changes

Changes introduced in this pull request:

Reference issue to close (if applicable)

Closes

Other information and links

Change checklist

  • I have performed a self-review of my own code,
  • I have made corresponding changes to the documentation. All new code adheres to the team's documentation standards,
  • I have added tests that prove my fix is effective or that my feature works (if possible),
  • I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

Summary by CodeRabbit

  • Refactor
    • Simplified internal message pool trigger handling by removing unnecessary abstraction layers, improving code maintainability without impacting user-facing functionality.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 8, 2025 •
edited
Loading

Walkthrough

Removes Arc wrapping from the republish trigger sender in the message pool module. The repub_trigger type changes from Arc<flume::Sender<()>> to flume::Sender<()>, with corresponding updates across the module's public API, internal implementations, and test setups.

Changes

Cohort / File(s) Summary
Message pool core refactoring
src/message_pool/msgpool/mod.rs		 Updated head_change() function signature and MessagePool::new() constructor to use flume::Sender<()> instead of Arc<flume::Sender<()>>. Removed Arc import. Updated all constructor calls to use Default::default() in place of Arc::default().
Message pool implementation
src/message_pool/msgpool/msg_pool.rs		 Changed repub_trigger cloning from Arc-wrapped to direct clone within the head-change handling block.
Test code
src/message_pool/msgpool/selection.rs		 Updated test setup to initialize repub_trigger with direct clone instead of Arc::new() wrapping.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • The changes follow a consistent, mechanical refactoring pattern across all files
  • Type simplification (removing Arc layer) with straightforward call site updates
  • Verify that flume::Sender supports direct cloning without Arc and that no other code paths depend on Arc-wrapped behavior

Possibly related PRs

Suggested reviewers

  • elmattic
  • akaladarshi
  • LesnyRumcajs

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: removing Arc wrapping from Sender types across multiple files in the message pool module.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch hm/no-arc-around-sender
📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 522e65a and 5c8e722.

📒 Files selected for processing (3)
  • src/message_pool/msgpool/mod.rs (8 hunks)
  • src/message_pool/msgpool/msg_pool.rs (1 hunks)
  • src/message_pool/msgpool/selection.rs (2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings 
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5867
File: src/ipld/util.rs:461-487
Timestamp: 2025-08-08T12:11:55.266Z
Learning: Forest (src/ipld/util.rs, Rust): In UnorderedChainStream::poll_next, dropping `extract_sender` (when no more tipsets and the extract queue is empty) is the intended shutdown signal for workers. Any subsequent attempt to enqueue work after this drop is a logic error and should be treated as an error; do not change `send()` to ignore a missing sender.

Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6074
File: src/rpc/methods/chain.rs:55-56
Timestamp: 2025-09-17T11:32:44.185Z
Learning: In the Forest codebase, hanabi1224 prefers that CodeRabbit should not warn about potential compilation issues (such as Send bounds, async/await compatibility, etc.) since they are already guarded by CI tests. Focus should be on higher-level design and logic issues instead.

Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5930
File: build.rs:64-77
Timestamp: 2025-08-13T09:43:20.301Z
Learning: hanabi1224 prefers hard compile-time errors in build scripts rather than runtime safeguards or collision detection, believing it's better to fail fast and fix root causes of issues like malformed snapshot names.

Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6057
File: src/cli/subcommands/f3_cmd.rs:0-0
Timestamp: 2025-09-09T10:37:17.947Z
Learning: hanabi1224 prefers having default timeouts (like 10m for --no-progress-timeout) to prevent commands from hanging indefinitely, even when the timeout flag isn't explicitly provided by users. This fail-fast approach is preferred over requiring explicit flag usage.
📚 Learning: 2025-08-08T12:11:55.266Z 
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5867
File: src/ipld/util.rs:461-487
Timestamp: 2025-08-08T12:11:55.266Z
Learning: Forest (src/ipld/util.rs, Rust): In UnorderedChainStream::poll_next, dropping `extract_sender` (when no more tipsets and the extract queue is empty) is the intended shutdown signal for workers. Any subsequent attempt to enqueue work after this drop is a logic error and should be treated as an error; do not change `send()` to ignore a missing sender.

Applied to files:

  • src/message_pool/msgpool/msg_pool.rs
  • src/message_pool/msgpool/mod.rs
🧬 Code graph analysis (1)
src/message_pool/msgpool/mod.rs (1)
src/message_pool/msgpool/msg_pool.rs (3)
  • flume (490-490)
  • new (72-77)
  • new (465-583)
⏰ 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). (7)
  • GitHub Check: Build forest binaries on Linux AMD64
  • GitHub Check: tests
  • GitHub Check: tests-release
  • GitHub Check: cargo-publish-dry-run
  • GitHub Check: All lint checks
  • GitHub Check: Build MacOS
  • GitHub Check: Build Ubuntu
🔇 Additional comments (6)
src/message_pool/msgpool/selection.rs (1)

908-908: LGTM: Simplified repub_trigger cloning.

The direct clone of repub_trigger correctly reflects the type change from Arc<flume::Sender<()>> to flume::Sender<()>. Since flume::Sender is already internally reference-counted and cheaply cloneable, the Arc wrapper was redundant.

Also applies to: 946-946

src/message_pool/msgpool/msg_pool.rs (1)

516-516: LGTM: Correctly updated for simplified repub_trigger type.

The direct clone is appropriate now that repub_trigger is a flume::Sender<()> rather than Arc<flume::Sender<()>>. This maintains the same functionality while removing an unnecessary layer of indirection.

src/message_pool/msgpool/mod.rs (4)

12-12: LGTM: Import cleanup.

Correctly removed the unused Arc import since repub_trigger no longer requires Arc wrapping.

216-216: LGTM: Simplified function signature.

The head_change function signature correctly updated to accept flume::Sender<()> directly instead of Arc<flume::Sender<()>>. This is a good simplification since flume::Sender is already designed to be cheaply cloneable and thread-safe.

359-366: LGTM: Test code consistently updated.

All test instantiations of MessagePool::new are correctly updated to work with the new parameter types using Default::default(). The changes are consistent across all test cases.

Also applies to: 445-452, 512-519, 611-618

473-473: LGTM: Direct cloning is appropriate.

The direct clone of mpool.repub_trigger is correct and consistent with the simplified type. This maintains the same functionality while removing unnecessary Arc indirection.

Also applies to: 540-540

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

@hanabi1224 hanabi1224 marked this pull request as ready for review December 8, 2025 00:21
@hanabi1224 hanabi1224 requested a review from a team as a code owner December 8, 2025 00:21
@hanabi1224 hanabi1224 requested review from LesnyRumcajs and akaladarshi and removed request for a team December 8, 2025 00:21
@LesnyRumcajs LesnyRumcajs added this pull request to the merge queue Dec 8, 2025
Merged via the queue into main with commit 4d47058 Dec 8, 2025
46 checks passed
@LesnyRumcajs LesnyRumcajs deleted the hm/no-arc-around-sender branch December 8, 2025 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@LesnyRumcajs LesnyRumcajs LesnyRumcajs approved these changes

@akaladarshi akaladarshi Awaiting requested review from akaladarshi akaladarshi is a code owner automatically assigned from ChainSafe/forest

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@hanabi1224 @LesnyRumcajs