peer: send reestablish, shutdown messages before starting writeHandler #8186
Conversation
Crypt-iQ
force-pushed
the
issue_8184_2
branch
from
7986a59
to
94e5402
Compare
This is to avoid a potential race on WriteMessage and Flush internals. Because there is no locking on WriteMessage and Flush, if we allow writeMessage calls in Start after the writeHandler has started, the writeMessage calls may call WriteMessage/Flush at the same time that writeMessage calls from the writeHandler does. Since there is no locking, internals like b.nextHeaderSend can race and cause panics.
Crypt-iQ
force-pushed
the
issue_8184_2
branch
from
94e5402
to
7556402
Compare
| @@ -703,6 +703,23 @@ func (p *Brontide) Start() error { | |||
|
|
|||
| p.startTime = time.Now() | |||
|
|
|||
| // Before launching the writeHandler goroutine, we send any channel | |||
There was a problem hiding this comment.
If we abstract out loadActiveChannels, or split it into: load chans and load messages, then we can write a unit test here to try to trigger the panic. IIUC, it relies on concurrent access to the brontide state machine, so the two goroutines need to line up directly. This may be easier to trigger with the -race flag on.
There was a problem hiding this comment.
I have a unit test that detects the race. Will send a PR once I clean it up.
|
Will make the release branch, merge this in, and add release notes. Goal here is a fast follow minor release with just this fix, if we can tighten up uni tests here than that's also ideal. Hopefully we're finally putting this saga to bed, and can devote resources to refactoring/re-writing this portion to get away from the mutex issues that started the saga in the first place. |
This is to avoid a race on
WriteMessageandFlushinternals. Because there is no locking onWriteMessageorFlush, if we allowwriteMessagecalls inStartafter thewriteHandlerhas started, thewriteMessagecalls may callWriteMessage/Flushat the same time thatwriteMessagecalls from thewriteHandlerdoes. Since there is no locking, internals likeb.nextHeaderSendcan race and cause panics.Fixes #8184