Fix race in syscallWatcher #431
Merged
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
Signed-off-by: Yechiel Kalmenson <ykalmenson@pivotal.io>
|
|
|
Thank you this is great! |
|
LGTM. @kevpar - PTAL |
kevpar
approved these changes
Dec 18, 2018
internal/hcs/watcher.go
Outdated
| // go syscallWatcher(context, &completed) | ||
| // <syscall> | ||
| // completed = true | ||
| // sysycallWatcher(logContext, func() { |
|
|
||
| func watchFunc(ctx context.Context, logContext logrus.Fields) { | ||
| select { | ||
| case <-ctx.Done(): |
There was a problem hiding this comment.
I don't think we need a select statement here, since there is only one case.
There was a problem hiding this comment.
That is correct that with a single case the code can be written inline. But this is a golang pattern so it doesn't bother me either way. Up to you
There was a problem hiding this comment.
Yeah, that's the pattern we've seen in the documentation. Let up know if you want us to change it.
arjun024
pushed a commit
to cloudfoundry/winc
that referenced
this pull request
Jan 29, 2019
Many transitive dependencies have been updated. This update includes our [PR](microsoft/hcsshim#431) to the syscall watcher All the tests pass with no major API changes Signed-off-by: Arjun Sreedharan <asreedharan@pivotal.io>
arjun024
pushed a commit
to cloudfoundry/winc
that referenced
this pull request
Jan 29, 2019
Uses greenhouse-org/hcsshim, branch upstream-v0.8.5-mod. This contains all commits from Microsoft/hcsshim v.0.8.5 + commits made by our team to the fork. Many transitive dependencies have been updated. This update includes our PR (microsoft/hcsshim#431) to the syscall watcher All the tests pass with no major API changes Signed-off-by: Arjun Sreedharan <asreedharan@pivotal.io>
arjun024
pushed a commit
to cloudfoundry/winc
that referenced
this pull request
Feb 5, 2019
Uses greenhouse-org/hcsshim, branch upstream-v0.8.5-mod. This contains all commits from Microsoft/hcsshim v.0.8.5 + commits made by our team to the fork. Many transitive dependencies have been updated. This update includes our PR (microsoft/hcsshim#431) to the syscall watcher All the tests pass with no major API changes Signed-off-by: Arjun Sreedharan <asreedharan@pivotal.io>
princepereira
pushed a commit
to princepereira/hcsshim
that referenced
this pull request
Aug 29, 2024
Fix race in syscallWatcher
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.
This PR fixes two issues:
If the syscallWatcher function is started in a goroutine, the goroutine remains around for the length of
timeout.SyscallWatcher(4 minutes). So a longrunning process can accumulate goroutines if it makes many syscalls that use the syscallWatcher in a 4 minute span.There is a race condition where both the syscallWatcher goroutine and the main function access the
completedvariable. This will cause any test suites that:go test -raceto fail due to the race detector. An easy way to reproduce is:
This PR fixes the issue by using a context.Context object to handle the timeout
Signed-off-by: Yechiel Kalmenson ykalmenson@pivotal.io