Skip to content

chore: document WebUIHandler thread-safety#297

Open
janechu wants to merge 2 commits into
mainfrom
users/janechu/document-webui-handler-thread-safety
Open

chore: document WebUIHandler thread-safety#297
janechu wants to merge 2 commits into
mainfrom
users/janechu/document-webui-handler-thread-safety

Conversation

@janechu
Copy link
Copy Markdown
Contributor

@janechu janechu commented May 16, 2026

What

Three small additions stating that WebUIHandler is Send + Sync:

  1. crates/webui-handler/src/lib.rs - expand the doc comment on WebUIHandler with a "Thread safety" section explaining why it's Send + Sync and the Arc<WebUIHandler> pattern.
  2. docs/guide/integrations/rust.md - new "Thread safety" section showing the same Arc example, so adopters following the integration page don't have to read the rustdoc.
  3. crates/webui-handler/src/lib.rs - compile-time assert_send_sync::<WebUIHandler>() inside the existing #[cfg(test)] module to fail-fast if a future field breaks Send or Sync.

Why

I'm wiring a WebUIHandler into a Tokio/Axum-shaped request path in an embedded host. The first question is "can I store one of these in axum::extract::State and serve concurrent requests from it, or do I need a per-task instance or a mutex?". Today, that question is answered only indirectly:

  • The existing doc says "stateless ... allowing concurrent renders with &self" - which strongly implies Send + Sync, but doesn't say so.
  • WebUIHandler has a single private field (Option<fn() -> Box<dyn HandlerPlugin>>), so the auto-traits do hold - but an adopter can't see the field from the public API and has to either reason about it from the struct definition or try compiling and find out.

Stating Send + Sync explicitly in both the rustdoc and the integration page removes that lookup. The static assertion keeps the docs honest if someone later adds a RefCell / Rc field.

How I tested

cargo check -p microsoft-webui-handler --tests

(passes - the assert_send_sync block compiles, confirming WebUIHandler: Send + Sync today.)

Notes

@janechu janechu force-pushed the users/janechu/document-webui-handler-thread-safety branch 2 times, most recently from 210df33 to df6cbf0 Compare May 16, 2026 14:46
@janechu janechu marked this pull request as ready for review May 16, 2026 14:46
WebUIHandler is auto-derived Send + Sync (its only field is a function
pointer plugin factory), and the existing doc comment already says
"plugin instances are created per-render ... allowing concurrent
renders with &self". But neither the type-level docs nor the rust
integration page state Send + Sync explicitly, so an adopter has to
either read the field list and reason about it themselves, or try
sharing one across threads and hope the compiler agrees.

This PR:

- Expands the doc comment on `WebUIHandler` with a "Thread safety"
  section that states `Send + Sync`, explains why (function-pointer
  factory + per-render local context), and shows the
  `Arc<WebUIHandler>` sharing pattern.

- Adds a `Thread safety` section to `docs/guide/integrations/rust.md`
  with the same `Arc` example so adopters following the integration
  page don't have to read the rustdoc.

- Adds a compile-time `assert_send_sync::<WebUIHandler>()` inside
  the existing `#[cfg(test)]` module. If a future field breaks Send
  or Sync, the build fails immediately and prompts an update to
  both the type and the docs.

No behavior change; docs + one compile-time assertion.
@janechu janechu force-pushed the users/janechu/document-webui-handler-thread-safety branch from df6cbf0 to d4d6bb2 Compare May 16, 2026 17:51
Copy link
Copy Markdown
Contributor

@mohamedmansour mohamedmansour left a comment

Choose a reason for hiding this comment

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

WebUIHandler is already thread-safe. Current code has handle(&self), creates a fresh per-render WebUIProcessContext, and creates plugin instances per render from a factory. Concurrent renders can run in parallel with no Mutex.

I see some issues, The added rustdoc says per-render state is created inside render, but the method is handle; there is no Self::render. You need to change [render](Self::render) to [handle](Self::handle).

Another issue DESIGN.md still contradicts the thread-safety contract. Update DESIGN.md to match current code: factory function field, with_plugin(factory: fn() -> Box<dyn HandlerPlugin>), and handle(&self, ...), plus the explicit Send+Sync/thread-safe contract.

PR should say “already thread-safe” and update the stale spec/incorrect rustdoc link before merging.

…ndler API

Addresses review feedback on #297:

- Rustdoc on `WebUIHandler` now links to `Self::handle` (the canonical
  documented entry point used in the integration page) instead of
  `Self::render`. Reframes 'is Send + Sync' as 'is already Send + Sync'
  to make clear this PR documents an existing property, not a new one.
- Drops the broken private intra-doc link to `WebUIProcessContext`
  (the struct is private; rustdoc emitted a private_intra_doc_links
  warning). Replaced with plain code text.
- DESIGN.md handler section updated to match current code:
    * `plugin: Option<Box<dyn HandlerPlugin>>`
      -> `plugin_factory: Option<fn() -> Box<dyn HandlerPlugin>>`
    * `with_plugin(plugin: Box<...>)`
      -> `with_plugin(factory: fn() -> Box<...>)`
    * `handle(&mut self, ...)` -> `handle(&self, ...)`
    * New 'Thread safety' subsection making the Send + Sync /
      Arc-without-Mutex contract explicit in the spec.
- DESIGN.md partial/action/template free-function signatures updated
  to match `route_handler.rs` (they no longer take a handler argument,
  which was the same staleness that contradicted the thread-safety
  story by implying `&mut WebUIHandler` was required).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@janechu
Copy link
Copy Markdown
Contributor Author

janechu commented May 19, 2026

Thanks for the review! Fixed in fbe0e3de. Summary of changes:

Rustdoc link (crates/webui-handler/src/lib.rs)
You're right that handle is the canonical documented entry point — the integration page and the rest of the rustdoc all use it. Changed both [render](Self::render) references to [handle](Self::handle). (For the record, a public render() method does still exist on WebUIHandler — it's the writer-doesn't-call-.end() variant of handle() — but routing the docs through handle is clearly the right framing, so I went with your suggestion.) Also reworded to "is already Send + Sync" so the framing is unambiguous that this PR documents an existing property.

While I was in the doc-comment I also dropped a broken intra-doc link: the original additions linked to [WebUIProcessContext], which is a private type, so cargo doc emitted a private_intra_doc_links warning. Replaced with plain code text.

DESIGN.md (Handler Implementation section)
Updated to match current code:

  • struct field: plugin: Option<Box<dyn HandlerPlugin>>plugin_factory: Option<fn() -> Box<dyn HandlerPlugin>>
  • with_plugin(plugin: Box<dyn HandlerPlugin>)with_plugin(factory: fn() -> Box<dyn HandlerPlugin>)
  • handle(&mut self, ...)handle(&self, ...)
  • New "Thread safety" subsection making the Send + Sync / Arc-without-Mutex contract explicit in the spec.

I also fixed the render_partial/render_action_response/render_component_templates signatures further down in the same section while I was in there — they were stale in the same way (showed handler: &mut WebUIHandler as the first parameter, which contradicts the thread-safety story; the actual functions in route_handler.rs don't take a handler at all). Happy to drop that part of the change if you'd prefer to keep this PR strictly scoped, but it felt tightly coupled to the same fix.

Verified locally:

  • cargo fmt --all --check
  • cargo clippy --workspace -- -D warnings
  • cargo xtask license-headers
  • cargo doc -p microsoft-webui-handler --no-deps ✔ (zero warnings)
  • cargo test -p microsoft-webui-handler --lib ✔ (285 passed, including the new assert_send_sync::<WebUIHandler>() compile-time check)

@janechu janechu requested a review from mohamedmansour May 19, 2026 21:31

The realistic pattern is "construct once, clone into many tasks":

```rust
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we need an example for thread safety, the docs state thread safety, we don't need more doc bloat showcasing how we can run the handler N times

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants