Skip to content
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.

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

Remove the Mutex from mempool::Crawler #2657

Closed
wants to merge 3 commits into from

Conversation

teor2345
Copy link
Contributor

Motivation

Here's one way to remove the Mutex from mempool::Crawler, and still spawn it in a separate task.

Solution

  • always take exclusive &mut self references in mempool::Crawler methods

Review

@jvff can use whatever bits of this PR he likes.

@teor2345 teor2345 added A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement P-Medium labels Aug 23, 2021
@teor2345 teor2345 requested a review from jvff August 23, 2021 07:51
@teor2345 teor2345 self-assigned this Aug 23, 2021
_ => unreachable!("Peer set did not respond with transaction IDs to mempool crawler"),
};
/// Handle a peer's response to the crawler's request for transactions.
async fn handle_response(response: Response) -> Result<(), BoxError> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't take &mut self here without cloning the crawler.
And generic type inference fails if it's a static method.

We'll probably just end up sending these responses to a channel anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I think I'm not a fan of having this method become a function. To me it seems easier to have it as a method because it will need to use the channel that's likely to be stored inside the Crawler type.

Wouldn't it be simpler to just wait until we have updated the Tokio version, and see if that fixes the issue in a more straightforward manner?

I'm okay if you decide this is a better way to structure things, I'm just trying to figure out what's the best way forward 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I think I'm not a fan of having this method become a function. To me it seems easier to have it as a method because it will need to use the channel that's likely to be stored inside the Crawler type.

Ok, I've changed it to a static method in commit 509f1fc.
And added a stub channel, to make sure that will all work.

What do you think?

Wouldn't it be simpler to just wait until we have updated the Tokio version, and see if that fixes the issue in a more straightforward manner?

I realise this question is a bit out of date - we're going to do the tokio upgrade after the mempool deadline.

I think having an instance method will work, but the ownership is a bit tricky to get right.
For an example, see PR #2661

We'll need to make sure all the fields are Clone and Sync.
But by the time we get around to that refactor, we'll have all the fields in place, so we'll know if that's reasonable.

Do you want to open a ticket for the post-tokio-upgrade refactor?

@teor2345
Copy link
Contributor Author

The original PR got merged.

@teor2345 teor2345 closed this Aug 24, 2021
@jvff jvff mentioned this pull request Sep 8, 2021
3 tasks
@teor2345 teor2345 deleted the mempool-crawler-no-mutex branch December 14, 2021 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants