NRG: Wait for goroutines to shutdown when recreating group #5832
Conversation
neilalexander
changed the title
neilalexander
force-pushed
the
neil/nrgwg
branch
from
9613916
to
01a15f8
Compare
neilalexander
changed the title
neilalexander
force-pushed
the
neil/nrgwg
branch
from
01a15f8
to
0ec0f43
Compare
neilalexander
changed the title
neilalexander
force-pushed
the
neil/nrgwg
branch
6 times, most recently
from
61b9a2e
to
3cc6288
Compare
neilalexander
force-pushed
the
neil/nrgwg
branch
from
3cc6288
to
751dec0
Compare
neilalexander
changed the title
|
Have rebased, this one is ready for review. |
| if node := s.lookupRaftNode(rg.Name); node != nil { | ||
| if node.State() == Closed { |
There was a problem hiding this comment.
Feels like there may be a better approach here.
Also I think we need to check that the peer lists are the same if we are going to re-use.
There was a problem hiding this comment.
Do we want to return a closed node here?
| // Wait for goroutines to shut down before trying to clean up | ||
| // the log below, otherwise we might end up deleting the store | ||
| // from underneath running goroutines. | ||
| n.Unlock() |
There was a problem hiding this comment.
We have these patterns for sure elsewhere but I try hard to avoid them if at all possible.
There was a problem hiding this comment.
I don't think we can avoid it here, it's the only way we can potentially avoid a deadlock if the runAs goroutine is finishing up processing some operations.
neilalexander
force-pushed
the
neil/nrgwg
branch
3 times, most recently
from
03b9f1d
to
4a02a47
Compare
neilalexander
force-pushed
the
neil/nrgwg
branch
from
4a02a47
to
566c9cc
Compare
This should fix some logical races where multiple sets of goroutines can fight over the same store directory, i.e. when shutting down and recreating a group rapidly. Signed-off-by: Neil Twigg <[email protected]>
Signed-off-by: Neil Twigg <[email protected]>
neilalexander
force-pushed
the
neil/nrgwg
branch
from
566c9cc
to
dcd8167
Compare
| if node := s.lookupRaftNode(rg.Name); node != nil { | ||
| if node.State() == Closed { |
There was a problem hiding this comment.
Do we want to return a closed node here?
| @@ -1728,12 +1731,17 @@ func (n *raft) Stop() { | |||
| n.shutdown(false) | |||
| } | |||
|
|
|||
| func (n *raft) WaitForStop() { | |||
| n.wg.Wait() | |||
There was a problem hiding this comment.
Should we check that the state is set to stopped to avoid calling in here and never returning?
| @@ -1744,12 +1752,12 @@ func (n *raft) shutdown(shouldDelete bool) { | |||
| if n.state.Swap(int32(Closed)) == int32(Closed) { | |||
| // If we get called again with shouldDelete, in case we were called first with Stop() cleanup | |||
| if shouldDelete { | |||
| n.wg.Wait() | |||
There was a problem hiding this comment.
Since we hold the lock here this may never return.
This should fix some logical races where multiple sets of goroutines can fight over the same store directory, i.e. when shutting down and recreating a group rapidly.
Signed-off-by: Neil Twigg [email protected]