Repository navigation
bft: abort the view directly instead of through the controller loop - #744
Conversation
| c.Batcher.Close() | ||
|
|
||
| c.abortViewChan <- view | ||
| c.abortView(view) |
There was a problem hiding this comment.
This can still return while a view is running. If the controller loop is in changeView and has aborted the old view but not yet called startView, AbortView aborts the already-stopped old view and returns, and then changeView starts a running view. I reproduced it by blocking NewProposer between the two steps. On main the queued abort ran after changeView, so it hit the new view. Could changeView and AbortView share a lock, like viewChangeLock in #741?
(Written by Claude Code.)
| currViewNum := c.getCurrentViewNumber() | ||
| c.Logger.Debugf("view for abort %d, current view %d", view, currViewNum) | ||
|
|
||
| if view < currView { | ||
| c.Logger.Debugf("Was asked to abort view %d but the current view with number %d", view, currView) | ||
| if view < currViewNum { | ||
| c.Logger.Debugf("Was asked to abort view %d but the current view with number %d", view, currViewNum) | ||
| return false | ||
| } | ||
|
|
||
| c.currViewLock.RLock() | ||
| currView := c.currView | ||
| c.currViewLock.RUnlock() | ||
|
|
There was a problem hiding this comment.
currViewNum and currView are read under separate locks, and changeView writes them in separate steps (setCurrentViewNumber, then startView). So AbortView(N) can pass the view < currViewNum check and then abort view N+1 that the loop just started. The lock from the other comment would cover this too.
(Written by Claude Code.)
| func (guardTestBatcher) Closed() bool { return false } | ||
| func (guardTestBatcher) Reset() {} | ||
|
|
||
| type guardTestProposer struct { |
There was a problem hiding this comment.
The file name is left over from #741, where the guard was removed. Could it be named after what it tests? And a real View (viewEnded.Add(1), then viewEnded.Done()) could replace guardTestProposer.
(Written by Claude Code.)
| c.Logger.Debugf("Aborting current view with number %d", currViewNum) | ||
| currView.Abort() |
There was a problem hiding this comment.
Now that the view changer waits in View.Abort, could the wait take a cancel such as the view changer's stopChan? Reconfig holds consensusLock and waits in viewChanger.Stop(), and a view stuck in Complain waits on that lock, so I think shutdown can hang. Not reproduced.
(Written by Claude Code.)
e0b3bcb to
3698f24
Compare
|
fixed |
|
|
||
| // AbortView makes the controller abort the current view | ||
| func (c *Controller) AbortView(view uint64) { | ||
| // AbortView makes the controller abort the current view and waits until the |
There was a problem hiding this comment.
Nit: "abort the current view" reads as if view is ignored. Maybe "aborts the current view, unless the given view is older than it"?
(Written by Claude Code.)
| // AbortView aborts the given view and waits until it has stopped. It stops | ||
| // waiting when cancel is closed, so that stopping the view changer does not | ||
| // hang on a view that cannot finish. |
There was a problem hiding this comment.
Nit: the view can finish, it's just blocked on the reconfig lock. Maybe drop "on a view that cannot finish", or say "a view that waits for a lock held by a reconfiguration", as in the controller comment?
(Written by Claude Code.)
| // viewChangeLock serializes aborting a view with starting or stopping one. | ||
| // AbortView must not return while changeView is between aborting the old | ||
| // view and starting the new one: the abort would hit the already stopped | ||
| // old view and the new view would keep running. The lock also keeps the | ||
| // view number and the view consistent, since changeView sets them in | ||
| // separate steps. | ||
| viewChangeLock sync.Mutex |
There was a problem hiding this comment.
Nit: viewChangeLock sounds like the view change protocol, but it guards swapping and aborting the current view in the controller. Maybe viewSwapLock or viewTransitionLock?
(Written by Claude Code.)
AbortView queued the request on abortViewChan and returned before the controller loop processed it. The view changer then wrote its record while the view was still running and could append a commit after it. Call abortView from AbortView, so AbortView returns only after View.Abort has stopped the view. Serialize changeView and AbortView with viewChangeLock, so AbortView cannot return between the old view abort and the new view start, and read currViewNumber and currView consistently under that lock. Let the abort wait stop when the view changer stops, so a view stuck behind the reconfiguration lock cannot hang viewChanger.Stop. Complain no longer waits for consensusLock during a reconfiguration, because the view changer it would notify is being replaced. Signed-off-by: Fedor Partanskiy <fredprtnsk@gmail.com>
3698f24 to
f7e35ae
Compare
|
fixed |
AbortView queued the request on abortViewChan and returned before the controller loop processed it. The view changer then wrote its record while the view was still running and could append a commit after it.
Call abortView from AbortView, so AbortView returns only after View.Abort has stopped the view. Read currView under currViewLock and handle a nil view, because the call now runs on the view changer goroutine.
#736