Control the broadcast channel lifecycle in a separate goroutine #3422
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
During the tests of the code orchestrating tECDSA signing, we realized some buffers get full and the broadcast channel keeps writing to them even though the receiver is no longer alive. This was fixed by introducing an additional context in the asynchronous state machine that unregisters the handler if the state machine (receiver) exits sooner than the context. This has fixed the problem on the receiver side but we still need to fix the problem on the producer side. And this is what this commit is doing. A separate goroutine controls the lifecycle of the handler. The message handler is removed from the channel if the context is done. This logic is placed in a separate goroutine because the call to `handleWithRetransmission` is blocking and we do not want to wait with removing the handler if that call blocks for a longer period of time, especially, when the underlying buffered channel is full.
Left a TODO about goroutine that may possibly be blocked forever. This shouldn't be a problem right now with how Recv consumers are constructed but this may become a problem in future. #3420 captures this work.
lukasz-zimnoch
approved these changes
Nov 29, 2022
lukasz-zimnoch
added a commit
that referenced
this pull request
Nov 29, 2022
Closes #3419 Depends on #3422 During the tests of the code orchestrating tECDSA signing in, we noticed the announcer's buffer sometimes gets full and the messages are dropped with the famous "message handler is too slow" warning. It turns out that the problem lies in the size of the buffer used by the announcer. Before a message in which the announcer is not interested is dropped, it is buffered in this channel. This also includes retransmissions from the previous signing protocols because the retransmission cache filter is scoped to the given `Recv` handler: ``` func WithRetransmissionSupport(delegate func(m net.Message)) func(m net.Message) { mutex := &sync.Mutex{} cache := make(map[string]bool) // (...) } ``` The announcer's buffer size has been increased to 512 elements, just like the buffer of the asynchronous state machine, and the problem seems to be gone based on local machine tests.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refs #3419
During the tests of the code orchestrating tECDSA signing in #3404, we realized some buffers get full and the broadcast channel keeps writing to them even though the receiver is no longer alive. This was fixed in #3418 by introducing an additional context in the asynchronous state machine that unregisters the handler if the state machine (receiver) exits sooner than the context. This has fixed the problem on the receiver side but we still need to fix the problem on the producer side. And this is what this PR is doing.
A separate goroutine controls the lifecycle of the handler. The message handler is removed from the channel if the context is done. This logic is placed in a separate goroutine because the call to
handleWithRetransmissionis blocking and we do not want to wait with removing the handler if that call blocks for a longer period of time, especially, when the underlying buffered channel is full.