fsevent should join on thread shutdown #337
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
We had some test failures because crossbeam-channel may panic when trying to call recv() during thread shutdown. This seems to be similar to this upstream bug: crossbeam-rs/crossbeam#321. Unfortunately it seems that some operating systems may tear down thread-local storage early, rust-lang/rust#28129, which can trigger panics if trying to interact with TLS during a drop. To avoid this issue, this switches from using a channel to signal the thread shutdown to just using the join handle (which we should have been doing anyway).
|
Also, I haven't yet figured out a workaround for this issue on Macs, so would it be possible to cut a release once this lands? |
0xpr03
approved these changes
Jun 15, 2021
Closed
gnoliyil
pushed a commit
to gnoliyil/fuchsia
that referenced
this pull request
Jan 27, 2024
This patch accounts for a number of complications we've discovered with the notify library. First, some notify backends may emit multiple notifications for the same event. For exampls, FSEvents on OSX will emit at least a create, modify-metadata, and modify-data events. Second, the event ordering can be somewhat non-deterministic. This can happen because the underlying notification system may also batch multiple events together. For example, on FSEevents, removing-then-recreating a file can emit a single event that has a bitfield that has a "delete" and "create" event. The notify library will unroll this into two separate events, but it doesn't have any way to tell the ordering of these events. To help deal with this, this patch creates a "notify-batch-watcher", which will batch up multiple events that occur over a period of time. We also drop the precise event kind. If it turns out we need this, we could add a bitflag field in the future. Finally, this refactors `PackageManifestWatcher` to use the helper library, and to simplify the tests to be quite a bit simpler. Note that this also removes the code accounting for notify-rs/notify#337, which was fixed in notify-5.0.0. Fixed: 117968 Change-Id: I3ac82308f38cb65e4b3d8bf5a50eacb55515f79b Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/794836 Reviewed-by: Gary Bressler <geb@google.com> Commit-Queue: Erick Tryzelaar <etryzelaar@google.com> Reviewed-by: Ben Keller <galbanum@google.com> Fuchsia-Auto-Submit: Erick Tryzelaar <etryzelaar@google.com>
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.
We had some test failures because crossbeam-channel may panic when trying to call recv() during thread shutdown in a drop(). This seems to be similar to this upstream bug: crossbeam-rs/crossbeam#321. This appears to be happening because recv() interacts with thread local storage, but unfortunately it seems that some operating systems may tear down thread-local storage early, rust-lang/rust#28129, which can trigger panics if trying to interact with TLS during a drop.
To avoid this issue, this switches from using a channel to signal the thread shutdown to just using the join handle (which we should have been doing anyway).