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

refactor Clique and StepService #10648

Closed
niklasad1 opened this issue May 13, 2019 · 1 comment · Fixed by #10683
Closed

refactor Clique and StepService #10648

niklasad1 opened this issue May 13, 2019 · 1 comment · Fixed by #10683
Labels
F6-refactor 📚 Code needs refactoring. P5-sometimesoon 🌲 Issue is worth doing soon.

Comments

@niklasad1
Copy link
Collaborator

niklasad1 commented May 13, 2019 •
edited
Loading

The issue is the following here

warning[E0382]: assign to part of moved value: `engine`
   --> ethcore/src/engines/clique/mod.rs:204:4
    |
190 |         let mut engine = Clique {
    |             ---------- move occurs because `engine` has type `engines::clique::Clique`, which does not implement the `Copy` trait
...
201 |         let res = Arc::new(engine);
    |                            ------ value moved here
...
204 |             engine.step_service = Some(StepService::start(Arc::downgrade(&res) as Weak<Engine<_>>));
    |             ^^^^^^^^^^^^^^^^^^^ value partially assigned here after move
    = warning: this error has been downgraded to a warning for backwards compatibility with previous releases
    = warning: this represents potential undefined behavior in your code and this warning will become a hard error in the future

To elaborate a bit, basically we mutate an already value moved into the Arc and this should not compile IMHO.

So, I would suggest to move the StepService into Clique instead because it is not safe/possible to mutate an Arc when it is aliased (i.e, more than 1 is pointing to it)

Thoughts?
@niklasad1 niklasad1 added F6-refactor 📚 Code needs refactoring. P5-sometimesoon 🌲 Issue is worth doing soon. labels May 13, 2019
@dvdplm
Copy link
Collaborator

dvdplm commented May 21, 2019

Yeah, seems like engine ends up in two Arcs, one assigned to step_service and the other returned by Clique::new? I'm also surprised it compiles tbh.

dvdplm added a commit that referenced this issue May 21, 2019
@dvdplm
Fix compiler warning (that will become an error)
ef48147 
Fixes #10648

I'm not sure this fix is Good™ but rustc seems happy enough.

There's a deeper issue which may or may not be related to this: the Engine is not shutdown properly and the `StepService` thread keeps running indefinitely after Ctrl-C (so `update_sealing()` is called repeatedly for 300sec). I don't think this is related to Clique as I've seen this happen on mainnet as well, but I wonder if the effects of it are worse for a PoA network where the node can create new blocks all on its own?
@dvdplm dvdplm mentioned this issue May 21, 2019
Merged
dvdplm added a commit that referenced this issue May 23, 2019
@dvdplm
Fix compiler warning (that will become an error) (#10683)
752031a 
* Remove annoying compiler warnings

* Fix compiler warning (that will become an error)

Fixes #10648

I'm not sure this fix is Good™ but rustc seems happy enough.

There's a deeper issue which may or may not be related to this: the Engine is not shutdown properly and the `StepService` thread keeps running indefinitely after Ctrl-C (so `update_sealing()` is called repeatedly for 300sec). I don't think this is related to Clique as I've seen this happen on mainnet as well, but I wonder if the effects of it are worse for a PoA network where the node can create new blocks all on its own?

* Use saturating_sub

* WIP

* Fix warning, second attempt

The idea here is to avoid using `Arc::get_mut()` (which does not work: fails every time here) and instead trust `drop()` to do the right thing.

This is a conservative change. I think this can be reformed further, e.g. by `impl Drop for StepService` and halt the thread there, or even skip `join()`ing the thread entirely and trust the `AtomicBool` to signal shutdown. I also have doubts abut the `Option<StepService>`: seems a bit much to have an `Option` there and it makes things cumbersome.
niklasad1 pushed a commit that referenced this issue Oct 8, 2019
@dvdplm @niklasad1
Fix compiler warning (that will become an error) (#10683)
30524b7 
* Remove annoying compiler warnings

* Fix compiler warning (that will become an error)

Fixes #10648

I'm not sure this fix is Good™ but rustc seems happy enough.

There's a deeper issue which may or may not be related to this: the Engine is not shutdown properly and the `StepService` thread keeps running indefinitely after Ctrl-C (so `update_sealing()` is called repeatedly for 300sec). I don't think this is related to Clique as I've seen this happen on mainnet as well, but I wonder if the effects of it are worse for a PoA network where the node can create new blocks all on its own?

* Use saturating_sub

* WIP

* Fix warning, second attempt

The idea here is to avoid using `Arc::get_mut()` (which does not work: fails every time here) and instead trust `drop()` to do the right thing.

This is a conservative change. I think this can be reformed further, e.g. by `impl Drop for StepService` and halt the thread there, or even skip `join()`ing the thread entirely and trust the `AtomicBool` to signal shutdown. I also have doubts abut the `Option<StepService>`: seems a bit much to have an `Option` there and it makes things cumbersome.
niklasad1 added a commit that referenced this issue Oct 23, 2019
@niklasad1
[stable]: backport #10691 and #10683 (#11143)
ebe4890 
* Fix compiler warning (that will become an error) (#10683)

* Remove annoying compiler warnings

* Fix compiler warning (that will become an error)

Fixes #10648

I'm not sure this fix is Good™ but rustc seems happy enough.

There's a deeper issue which may or may not be related to this: the Engine is not shutdown properly and the `StepService` thread keeps running indefinitely after Ctrl-C (so `update_sealing()` is called repeatedly for 300sec). I don't think this is related to Clique as I've seen this happen on mainnet as well, but I wonder if the effects of it are worse for a PoA network where the node can create new blocks all on its own?

* Use saturating_sub

* WIP

* Fix warning, second attempt

The idea here is to avoid using `Arc::get_mut()` (which does not work: fails every time here) and instead trust `drop()` to do the right thing.

This is a conservative change. I think this can be reformed further, e.g. by `impl Drop for StepService` and halt the thread there, or even skip `join()`ing the thread entirely and trust the `AtomicBool` to signal shutdown. I also have doubts abut the `Option<StepService>`: seems a bit much to have an `Option` there and it makes things cumbersome.

* Refactor Clique stepping (#10691)

* Use Drop to shutdown stepper thread
Make period == 0 an error and remove the Option from step_service

* Remove StepService

Remove StepService and spawn the stepping thread in `Clique::new()`. Don't store the thread handle and instead trust the `AtomicBool` to signal shutdown time.
Don't check for `period > 0`: we assume a valid chainspec file.

* Don't shutdown the stepper thread at all, just let it run until exit
Also: fix a few warnings and tests

* Put kvdb_memorydb back

* Warn&exit when engine is dropped
Don't sleep too long!

* Don't delay stepping thread

* Better formatting
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
F6-refactor 📚 Code needs refactoring. P5-sometimesoon 🌲 Issue is worth doing soon.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix compiler warning (that will become an error) openethereum/parity-ethereum
2 participants
@dvdplm @niklasad1