Skip to content

fix(request-sharing): Remove clone and fix gc task lifetime#4411

Open
metalurgical wants to merge 16 commits into
cowprotocol:mainfrom
metalurgical:request_sharig_clone_impl_fix
Open

fix(request-sharing): Remove clone and fix gc task lifetime#4411
metalurgical wants to merge 16 commits into
cowprotocol:mainfrom
metalurgical:request_sharig_clone_impl_fix

Conversation

@metalurgical

@metalurgical metalurgical commented May 14, 2026

Copy link
Copy Markdown
Contributor

Description

Remove Clone implementation for RequestSharing, allow the GC task to exit once Arc is dropped.

Changes

  • Remove clone implementation.
  • spawn_gc modified to check Arc and exit if dropped.

How to test

cargo nextest run -p request-sharing

…g independent one

Additionally, gc task exits when all handles dropped, drop only zeroes on last handle
@metalurgical metalurgical requested a review from a team as a code owner May 14, 2026 12:18

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the RequestSharing implementation to share the in-flight request cache across clones and modifies the garbage collection task to use a weak reference, ensuring the task terminates when all handles are dropped. A critical race condition was identified where the request_sharing_cached_items metric might remain at a non-zero value if the GC task holds a strong reference during the final drop of the cache handles. It is recommended to explicitly zero the metric within the GC task's exit path to ensure consistent state reporting.

Comment thread crates/request-sharing/src/lib.rs
Comment thread crates/request-sharing/src/lib.rs Outdated
Comment thread crates/request-sharing/src/lib.rs Outdated
Comment thread crates/request-sharing/src/lib.rs Outdated
@metalurgical metalurgical changed the title fix(request-sharing): correct clone, gc task lifetime, and drop zeroing for RequestSharing fix(request-sharing): remove clone and fix gc task lifetime Jun 9, 2026
@metalurgical metalurgical changed the title fix(request-sharing): remove clone and fix gc task lifetime fix(request-sharing): Remove clone and fix gc task lifetime Jun 9, 2026
@metalurgical metalurgical requested a review from MartinquaXD June 9, 2026 13:24
@MartinquaXD MartinquaXD added this pull request to the merge queue Jun 10, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 10, 2026
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