feat(axevent)!: Improve callbacks #59
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
The summary of this commit is poor because I am being lazy and changing two things. Box is removed from the type of subscription callbacks to make the interface nicer; the user does not have to type out `Box::new(...)` and there will generally be less indentation. One potential downside of this is that monomorphization of generic functions tend to increase the binary size and I suspect that callbacks are particularly bad since each callback gets its own type. The type of declaration callbacks is constrained to make avoiding leaking the callbacks easier. It is probably possible to convert the `FnOnce` to an `FnMut` the same way we have done in other places but because the argument is a `Box<dyn ...>` this is not as straightforward and since I'd like to remove the box from the interface entirely it is not a detour I want to take. The `CallOnDrop` type is introduced to make it easier to ensure that callbacks are freed exactly once. Though convenient I think this pattern seems dangerous since it is not immediately obvious where the unsafe code is being called.
Draft
The intended benefit of this commit is to make the API slightly more ergonomic by allowing users to omit `Box::new`. Possible side effects that I have not investigated include: - Better, worse or different compiler errors. - Larger binaries due to monomorphization. - Harder to package as rlib due to more generic methods. `apps/send_event/src/main.rs`: - Make the declaration callback `FnMut` even though I expect it to be called at most once because I have not confirmed this suspicion and implementing dropping it as `FnOnce` turned out to be difficult. - Add a test ensuring that it continues to be possible to provide no callback. The main benefit of this is failing to compile if there is a regression, but running it makes it show up in test summaries which is nice. `crates/axevent/src/flex.rs`: - Add the `Deferred` helper type to make dropping the callbacks easier now that they do not have a knowable type. - Guard the handlers with `Mutex` to keep the handler `Send` and `Sync` like the underlying C struct.
apljungquist
added a commit
that referenced
this pull request
Oct 15, 2024
…per_and_example * upstream/main: Update rust toolchain to 1.80.1 (#100) fix!: Make builds reproducible in dev-container (#99) feat(axevent)!: Use generic callbacks and remove leak (#59) fix: Install `cargo-acap-sdk` in venv (#97) chore: release cargo-acap-sdk 0.2.0 (#95) chore(deps): bump dirs from 3.0.2 to 5.0.1 (#85) Add `Cargo.lock` to generated files (#93) chore: Build apps with custom profile (#90) Remove leftover `dbg!` and copy `.eap` to appropriate names (#91) Use `cargo` with `--locked` option in checks (#92)
apljungquist
added a commit
that referenced
this pull request
Nov 5, 2024
* upstream/main: (48 commits) chore: Use workspace dependencies consistently (#126) fix(axevent_example): Return `Break` from signal handlers (#125) chore(deps): bump ghcr.io/devcontainers/features/common-utils (#124) feat(device-manager): Allow unsigned ACAPs (#122) feat(acap-build): Enable bypassing `acap-build` (#119) chore: streamline CI (#118) fix: Don't invalidate Cargo caches (#117) fix(acap-ssh-utils): Warn users that commands may break (#114) feat: Factor out `acap-build` wrapper to lib crate (#112) feat: Add `fleet-manager` program (#111) feat: Add edge storage wrapper and example (#108) fix: Measure apparent size (#107) chore(acap-logging): Bump version (#106) fix(cargo-acap-sdk): Set executable bits (#105) fix(acap-logging)!: Disable default features for env_logger (#103) chore: Track size of apps (#102) chore: Don't use clearly defined for license info (#101) Update rust toolchain to 1.80.1 (#100) fix!: Make builds reproducible in dev-container (#99) feat(axevent)!: Use generic callbacks and remove leak (#59) ...
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 summary of this commit is poor because I am being lazy and changing two things.
Box is removed from the type of subscription callbacks to make the interface nicer; the user does not have to type out
Box::new(...)and there will generally be less indentation. One potential downside of this is that monomorphization of generic functions tend to increase the binary size and I suspect that callbacks are particularly bad since each callback gets its own type.The type of declaration callbacks is constrained to make avoiding leaking the callbacks easier. It is probably possible to convert the
FnOnceto anFnMutthe same way we have done in other places but because the argument is aBox<dyn ...>this is not as straightforward and since I'd like to remove the box from the interface entirely it is not a detour I want to take.The
Deferredtype is introduced to make it easier to ensure that callbacks are freed exactly once. Though convenient I think this pattern seems dangerous since it is not immediately obvious where the unsafe code is being called.