Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

fix(clique step_service): remove needless Arc #10690

Closed
wants to merge 1 commit into from

Conversation

niklasad1
Copy link
Collaborator

No description provided.

@niklasad1 niklasad1 added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels May 22, 2019
@niklasad1 niklasad1 requested a review from dvdplm May 22, 2019 21:51
Copy link
Collaborator

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

So I think this is good stuff, but you need the changes from #10689 and even then stop() is not getting called because Drop for Client fails (here).

Curious though: how do you know the Arc isn't needed? I find it hard to prove that an Arc is not needed (sure, I guess in this case you know that no code passes the StepService to diff threads?). How do you go about it?

@niklasad1
Copy link
Collaborator Author

It shouldn't compile then because Clique is wrapped in an Arc directly in the constructor, which doesn't provide mutation unless risky Arc::get_mut(). Thus, I think stop() never is called otherwise it shouldn't compile.

Basically, this is all wrong either use drop or change to stop(&self) and wrap step_service in an Mutex/RwLock.

Closing in favor of #10683

@niklasad1 niklasad1 closed this May 23, 2019
@niklasad1 niklasad1 deleted the na-clique-stepservice-rm-arc branch May 23, 2019 15:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants