Make ExtensionManagerServer.Shutdown idempotent
#117
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
sharon-fdm
reviewed
Nov 7, 2023
| } | ||
| s.serverClient.Close() |
There was a problem hiding this comment.
Don't we need to close ?
(s.serverClient.Close())
There was a problem hiding this comment.
Huh. This line looks like I should have removed it in #112
3 tasks
directionless
approved these changes
Nov 7, 2023
| } | ||
| s.serverClient.Close() |
There was a problem hiding this comment.
Huh. This line looks like I should have removed it in #112
Comment on lines
+258
to
+260
| s.mutex.Lock() | ||
| serverClient := s.serverClient | ||
| s.mutex.Unlock() |
There was a problem hiding this comment.
I'm not totally sure I understand why this helps. I think it's so that this routine can happen at the same time some other thread might have called shutdown (and set s.serverClient to nil), but I'm not totally sure I see it.
Is this about a narrow race where shutdown happens between if serverClient == nil and s.serverClient.Ping() ?
There was a problem hiding this comment.
Yes, it's narrow.
Here's the race condition if we don't add the locks (it's between the read serverClient := s.serverClient and the write s.serverClient = nil in Shutdown):
=== RUN TestNoDeadlockOnError
==================
WARNING: DATA RACE
Write at 0x00c00010f610 by goroutine 14:
github.com/osquery/osquery-go.(*ExtensionManagerServer).Shutdown()
/Users/luk/fleetdm/git/forks/osquery-go/server.go:352 +0x3d2
github.com/osquery/osquery-go.(*ExtensionManagerServer).Run()
/Users/luk/fleetdm/git/forks/osquery-go/server.go:279 +0x1d0
github.com/osquery/osquery-go.TestNoDeadlockOnError()
/Users/luk/fleetdm/git/forks/osquery-go/server_test.go:57 +0x55c
testing.tRunner()
/Users/luk/go/src/testing/testing.go:1595 +0x238
testing.(*T).Run.func1()
/Users/luk/go/src/testing/testing.go:1648 +0x44
Previous read at 0x00c00010f610 by goroutine 16:
github.com/osquery/osquery-go.(*ExtensionManagerServer).Run.func2()
/Users/luk/fleetdm/git/forks/osquery-go/server.go:259 +0x64
Goroutine 14 (running) created at:
testing.(*T).Run()
/Users/luk/go/src/testing/testing.go:1648 +0x82a
testing.runTests.func1()
/Users/luk/go/src/testing/testing.go:2054 +0x84
testing.tRunner()
/Users/luk/go/src/testing/testing.go:1595 +0x238
testing.runTests()
/Users/luk/go/src/testing/testing.go:2052 +0x896
testing.(*M).Run()
/Users/luk/go/src/testing/testing.go:1925 +0xb57
main.main()
_testmain.go:69 +0x2bd
Goroutine 16 (running) created at:
github.com/osquery/osquery-go.(*ExtensionManagerServer).Run()
/Users/luk/fleetdm/git/forks/osquery-go/server.go:255 +0x18c
github.com/osquery/osquery-go.TestNoDeadlockOnError()
/Users/luk/fleetdm/git/forks/osquery-go/server_test.go:57 +0x55c
testing.tRunner()
/Users/luk/go/src/testing/testing.go:1595 +0x238
testing.(*T).Run.func1()
/Users/luk/go/src/testing/testing.go:1648 +0x44
==================
testing.go:1465: race detected during execution of test
--- FAIL: TestNoDeadlockOnError (0.00s)
directionless
approved these changes
Nov 8, 2023
lucasmrod
added a commit
to fleetdm/fleet
that referenced
this pull request
Nov 13, 2023
…#15017) #15022 The issue in the package is being fixed here osquery/osquery-go#117 But to not block on that we will downgrade the osquery-go version we use. - ~[ ] Changes file added for user-visible changes in `changes/` or `orbit/changes/`. See [Changes files](https://fleetdm.com/docs/contributing/committing-changes#changes-files) for more information.~ - ~[ ] Documented any permissions changes (docs/Using Fleet/manage-access.md)~ - ~[ ] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements)~ - ~[ ] Added support on fleet's osquery simulator `cmd/osquery-perf` for new osquery data ingestion features.~ - ~[ ] Added/updated tests~ - [ ] Manual QA for all new/changed functionality - ~For Orbit and Fleet Desktop changes:~ - [ ] Manual QA must be performed in the three main OSs, macOS, Windows and Linux. - [ ] Auto-update manual QA, from released version of component to new version (see [tools/tuf/test](../tools/tuf/test/README.md)).
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.
The change in dbeefc0 causes nil dereference panics when calling
ExtensionManagerServer.Shutdownmore than once.Here's a stack trace of the panic without the fix: