Skip to content

fix(nmrs): add Send bound to for_each_access_point callback future#330

Merged
cachebag merged 1 commit intocachebag:masterfrom
okhsunrog:pr/for-each-ap-send-bound
Apr 9, 2026
Merged

fix(nmrs): add Send bound to for_each_access_point callback future#330
cachebag merged 1 commit intocachebag:masterfrom
okhsunrog:pr/for-each-ap-send-bound

Conversation

@okhsunrog
Copy link
Copy Markdown
Contributor

@okhsunrog okhsunrog commented Apr 9, 2026

The Box<dyn Future<Output = ...> + 'a> in for_each_access_point's F: FnMut bound is missing a + Send clause. Every future that awaits this helper (transitively: list_networks, current_network_info, and the public NetworkManager methods that call them) ends up !Send, which blocks callers from running nmrs on a default multi-threaded tokio runtime:

dyn Future<Output = Result<Option<(Cow<'_, str>, u32, Network)>, ...>>
    cannot be sent between threads safely

This trips at every tokio::spawn site in any GUI/server/CLI app that uses #[tokio::main] (the default rt-multi-thread flavor).

The bound is always satisfiable:

  • NMAccessPointProxy is Send + Sync — zbus's #[proxy]-generated proxies are Send + Sync whenever the backing Connection is, which it is on both tokio and async-io runtimes in zbus 5.x.
  • The two in-tree callers — the async blocks inside core::scan::list_networks and monitoring::info::current_network_info — only capture Send values (Arc<str>, String, primitives) and only .await on proxy method calls whose outputs are all Send primitives (u8, u32, Vec<u8>, String, etc.).
  • Nothing across an .await holds an Rc, RefCell, MutexGuard, or any other well-known non-Send type.

Adding + Send to the helper's where-clause makes both existing callers Send-compatible with no call-site changes. All 86 tests and doctests in the workspace still pass. The crate now builds cleanly when consumed from a multi-threaded tokio runtime.

Context: discovered this while wiring nmrs into the archinstall_zfs installer GUI, which uses Slint on top of #[tokio::main] and spawns wifi work via tokio::spawn. Without this fix the crate is effectively unusable from default tokio setups.


AI disclosure (per CONTRIBUTING.md): this fix was authored with Claude Code. I hit the !Send error in my downstream project, described the symptom, and Claude read the nmrs source, located the missing bound in util/utils.rs::for_each_access_point, verified from the code that adding + Send was safe (proxy is Send + Sync, no non-Send captures), wrote the patch, and ran the full test suite to verify. I reviewed the diff before opening the PR.

@cachebag cachebag added bug Something isn't working feature New feature or request nmrs Changes to nmrs labels Apr 9, 2026
Copy link
Copy Markdown
Owner

@cachebag cachebag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this! LGTM!

If you don't mind, could you please just reword your commit to follow standards expressed in the contribution guidelines?

@cachebag cachebag changed the title Add Send bound to for_each_access_point callback future fix(nmrs): add Send bound to for_each_access_point callback future Apr 9, 2026
The Box<dyn Future + 'a> in the helper's where-clause lacks a
+ Send bound, making every downstream future !Send and blocking
callers that use the default multi-threaded tokio runtime. The
bound is always satisfiable (NMAccessPointProxy is Send + Sync,
captured values are all Send) so adding + Send is a no-op for
existing callers.

AI disclosure (per CONTRIBUTING.md): authored with Claude Code.
I hit the !Send error in a downstream project, described the
symptom, and Claude located the missing bound in util/utils.rs,
verified it was safe to add, wrote the patch, and ran the tests.
I reviewed the diff before opening the PR.
@okhsunrog okhsunrog force-pushed the pr/for-each-ap-send-bound branch from 97854e1 to 774c95c Compare April 9, 2026 17:07
@okhsunrog
Copy link
Copy Markdown
Contributor Author

@cachebag Thanks for the quick review! I edited the PR description and the commit message as described in the contributing guide.

One unrelated question while I'm here: the repo has a gitlink entry for nmrs-aur at the root (160000 mode, commit 72855e4a7b57), but there's no .gitmodules file defining its URL. This blocks using the repo as a cargo git dependency — cargo recurses into submodules by default and fails with:

no URL configured for submodule 'nmrs-aur'; class=Submodule (17)

I worked around it in my fork by removing the gitlink, but the proper fix is probably either adding a .gitmodules with the real URL or dropping the gitlink from master. Happy to file a separate issue/PR for that if you'd prefer — didn't want to bundle it into this PR since it's unrelated to the Send bound.

@cachebag
Copy link
Copy Markdown
Owner

cachebag commented Apr 9, 2026

One unrelated question while I'm here: the repo has a gitlink entry for nmrs-aur at the root (160000 mode, commit 72855e4a7b57), but there's no .gitmodules file defining its URL. This blocks using the repo as a cargo git dependency — cargo recurses into submodules by default and fails with...

Yes, this was left over from me trying to include the AUR repo as a submodule, which I have learned was quite useless. Never cleaned it up.

A follow up PR would be appreciated! Thanks for the heads up.

@cachebag cachebag enabled auto-merge (rebase) April 9, 2026 17:18
@cachebag cachebag disabled auto-merge April 9, 2026 17:19
@cachebag cachebag merged commit e1e5f5b into cachebag:master Apr 9, 2026
6 checks passed
@okhsunrog
Copy link
Copy Markdown
Contributor Author

@cachebag I also noticed that the AUR repo is either private or removed. Is there any reason for this?

@cachebag
Copy link
Copy Markdown
Owner

cachebag commented Apr 9, 2026

@cachebag I also noticed that the AUR repo is either private or removed. Is there any reason for this?

It's not private, I just haven't uploaded a mirror to GitHub yet. Yet another thing I should have done a long time ago but never did.

@okhsunrog
Copy link
Copy Markdown
Contributor Author

It's not private, I just haven't uploaded a mirror to GitHub yet. Yet another thing I should have done a long time ago but never did.

Classic story — created a project for personal use, uploaded it to GitHub, suddenly people start using it and complaining in issues :D

@cachebag
Copy link
Copy Markdown
Owner

cachebag commented Apr 9, 2026

It's not private, I just haven't uploaded a mirror to GitHub yet. Yet another thing I should have done a long time ago but never did.

Classic story — created a project for personal use, uploaded it to GitHub, suddenly people start using it and complaining in issues :D

Haha pretty much! I'm more than happy to address them though. It's quite heartwarming to see people use nmrs, so it's no bother to me at all to be caught slipping like this 😅

okhsunrog added a commit to okhsunrog/archinstall_zfs that referenced this pull request Apr 9, 2026
Both of our PRs on upstream nmrs have merged:
- cachebag/nmrs#330 (Send bound on for_each_access_point)
- cachebag/nmrs#331 (stale nmrs-aur gitlink removal)

The latest published crates.io release (2.2.0) still predates both,
so we stay on a git patch until a new version is cut — but the
patch now points at upstream master pinned to commit 352f57e
instead of our local fork branch.

The old fork branch `okhsunrog/nmrs#fix/send-bounds-for-each-ap`
becomes unreferenced after this; it can stay around as a safety
net or be deleted at any time since upstream has the same content.

Drop this [patch.crates-io] block entirely once nmrs cuts a new
release and bump core/Cargo.toml's nmrs constraint to it.

Verified all four slint-ui + tui profiles build clean against
the upstream pin (linuxkms, desktop, desktop-mock, tui) and
core wifi tests still pass.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working feature New feature or request nmrs Changes to nmrs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants