-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[bugfix] Integrate improved three-point lock for capture routines and improve concurrency patterns #318
[bugfix] Integrate improved three-point lock for capture routines and improve concurrency patterns #318
Conversation
…concurrency patterns
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The usual mimi. Otherwise: good to go!
status, err := mc.status() | ||
if lErr := mc.capLock.Unlock(); lErr != nil { | ||
logger := logging.FromContext(runCtx) | ||
logger.Errorf("failed to release status call three-point lock: %s", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: this is copy-pasta. Put into err messages.
logger := logging.FromContext(runCtx) | ||
logger.Errorf("failed to release status call three-point lock: %s", err) | ||
if err := mc.close(); err != nil { | ||
logger.Errorf("failed to close capture after failed three-point lock: %s", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
@@ -62,6 +63,51 @@ func (t testMockSrcs) Wait() error { | |||
return nil | |||
} | |||
|
|||
// func TestInterfaceChanges(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out with it? Or do you still need it?
First attempt at fixing (or at least improving) the potential deadlocks. I still wasn't able to reproduce the issue to its full extend, but at least one of the observed crashes I confirmed to be addressed by this. The issue will remain open after merging this for further assessment.