Skip to content

Conversation

@ruseinov
Copy link
Contributor

@ruseinov ruseinov commented Mar 2, 2024

Summary of changes

Changes introduced in this pull request:

  • implemented PinnedDrop for UnorderedChainStream to make sure we cleanup automatically.
  • avoid worker panics when UnorderedChainStream is dropped, there is no need to panic when the receiver channel is closed.
  • removed redundant join_workers method as we are cleaning up in Drop.
  • improved into_seen implementation to work properly with Drop and avoiding Arc nuances.

Reference issue to close (if applicable)

Closes #3991

Other information and links

Change checklist

  • I have performed a self-review of my own code,
  • I have made corresponding changes to the documentation,
  • 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.

@ruseinov ruseinov requested a review from a team as a code owner March 2, 2024 12:59
@ruseinov ruseinov requested review from hanabi1224 and sudo-shashank and removed request for a team March 2, 2024 12:59
match Arc::try_unwrap(self.seen) {
Ok(v) => v.into_inner(),
Err(v) => v.lock().clone(),
impl<DB, T> PinnedDrop for UnorderedChainStream<DB, T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't find doc for implementing PinnedDrop with pin-project-lite, have you verified drop is being called?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

@hanabi1224 hanabi1224 left a comment

Choose a reason for hiding this comment

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

The change LGTM. It will be great to add some more detailed description about what is improved or fixed by this PR

@ruseinov
Copy link
Contributor Author

ruseinov commented Mar 5, 2024

@lemmih perhaps we could also modify the into_seen in ChainStream to also use mem::swap?

@lemmih
Copy link
Contributor

lemmih commented Mar 5, 2024

@lemmih perhaps we could also modify the into_seen in ChainStream to also use mem::swap?

Sure. But I'm not sure I understand what is going on.

@ruseinov
Copy link
Contributor Author

ruseinov commented Mar 5, 2024

@lemmih perhaps we could also modify the into_seen in ChainStream to also use mem::swap?

Sure. But I'm not sure I understand what is going on.

Basically we couldn't move out of self due to Drop implementation now present, so I had to change that up for unordered implementation.

@ruseinov ruseinov requested a review from lemmih March 6, 2024 15:30
@ruseinov ruseinov added this pull request to the merge queue Mar 13, 2024
Merged via the queue into main with commit fd7b1a3 Mar 13, 2024
@ruseinov ruseinov deleted the ru/fix/sender-receiver-ucs branch March 13, 2024 18:48
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.

Failed to export diff snapshots

4 participants