[bugfix] Address several remaining race conditions to improve stability #325
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
fako1024
added
bug
3 tasks
fako1024
commented
Jun 11, 2024
| c.wgProc.Wait() | ||
| c.capLock.Close() |
There was a problem hiding this comment.
Uses the upstream improvement from gotools/concurrency...
pkg/capture/capture.go
Outdated
| @@ -347,14 +348,14 @@ func (c *Capture) bufferPackets(buf *LocalBuffer, captureErrors chan error) erro | |||
|
|
|||
| // Ensure that the buffer is released at the end of the method | |||
| defer func() { | |||
| c.capLock.ConsumeUnlockRequest() // Consume the unlock request to continue normal processing | |||
There was a problem hiding this comment.
Moved this into the defer to ensure it's being run (even if in the future code is added below). Since the function is on the stack anyway this has no negative impact.
| @@ -580,12 +583,12 @@ func (cm *Manager) performWriteout(ctx context.Context, timestamp time.Time, ifa | |||
| writeoutChan := make(chan capturetypes.TaggedAggFlowMap, writeout.WriteoutsChanDepth) | |||
| doneChan := cm.writeoutHandler.HandleWriteout(ctx, timestamp, writeoutChan) | |||
|
|
|||
| cm.Lock() | |||
There was a problem hiding this comment.
This was also incorrect. The global lock must encompass the whole (unprotected) rotation, otherwise it's possible that a scheduled one runs in parallel to one triggered from an interface down. TBH this is the most likely candidate for the deadlocks (because we've seen it happen in exactly these scenarios quite frequently - interface changes + scheduled rotation)....
fako1024
changed the title
|
Damn, now there's a race in the capture manager logic (probably has been there all along, just never got triggered): On it... |
…tions and introduces a new race condition
fako1024
commented
Jun 11, 2024
els0r
approved these changes
Jun 11, 2024
els0r
deleted the
317-deadlock-in-three-point-capture-routine-lock-sporadic-2
branch
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.
Improves multiple things across the board detected when running hacksaw-style concurrency tests using an actual deployment and
gpctl(excessively).Closes #317