Don't hold lock in input_intercept callback and don't expose xkb:: types in safe API
#1533
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
YaLTeR
reviewed
Sep 14, 2024
Drakulix
reviewed
Sep 18, 2024
|
Hopefully we can improve things here more in the future, since none of this seems perfect, but the changes here at least clean up confusing behavior in the API around mutex locking (and the technical soundness issue of providing safe xkb accessors) without any significant change to the API. |
It's cleaner to make the `unsafe impl Send` more fine-grained, and this should help with some future changes. Since `XkbContextHandler` is public, this fixes a soundness issue in the public API, since it was possible with a safe call to get an `xkb::Keymap` and `xkb::State`, and clone those. Potentially violating the safety assumption of the `Send` implementation.
Using composition like this seems like a cleaner solution than a trait with default implementations.
Drakulix
approved these changes
Sep 18, 2024
Holding a mutex during the callback causes a deadlock if something invoked by the callback tries to call a `KeyboardHandle` method. Or if the callback locks another mutex, which in another thread may be held while the keyboard mutex is locked. This is bad, and the mutex here should generally be a hidden implementation detail. Or if absolutely necessarily this restriction should be documented. This requirements to make this work aren't ideal, but probably aren't a problem. We have to create a copy of `mods_state` on the stack. `Xkb` needs to be store in an `Arc<Mutex<_>>` for finer-grained locking. And `modified_syms()` and `raw_syms()` need to return `Vec`s instead of slices. The `Vec`s could be changed back to slices in the type signatures if we used `OnceCell` to cache the return value, but that probably doesn't matter that much in practice.
No longer needed now that this isn't part of a trait.
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.
See commits messages for descriptions of the changes here. The last commit is potentially more controversial than the earlier ones; if it's an issue, I think the
Xkbchanges in the others are a good improvement at least.