forked from 1b-agent/opencadstudio-upstream
fix(plugins): degrade ensure_plugin_state instead of panicking
Bug:
The typed ensure_plugin_state helper and the object-safe trait method
it wraps turned two recoverable conditions into process deaths. On the
in-process host, the helper downcast the stored state with
.expect("plugin state type mismatch for plugin_id"), so registering
state under one plugin_id as one type and later asking for another
(e.g. a reloaded plugin version changes the type while the per-tab
state map survives) panicked the host. On out-of-process hosts
(ipc/client.rs, ipc/v4/client.rs) and on NullHost, the trait method
itself was implemented as a bare panic!("ensure_plugin_state is not
supported for out-of-process plugins..."), so any out-of-process
plugin calling the documented helper crashed its own runner process -
even though the sibling plugin_state_any and plugin_state_any_mut
methods on those same implementations already degrade to None
gracefully. The helper also used .expect("init called once") on the
initialiser closure.
Reproduce:
1. Out-of-process: a plugin whose runner calls ensure_plugin_state
(&mut host, "my.id", ...).
2. Type mismatch: in an in-process plugin, ensure state under one id
as u32, then ensure a different type under the same id.
3. Before: step 1 kills the plugin runner at the trait panic; step 2
kills the host at the downcast expect. After: both get None, keep
running, and existing state is untouched.
Fix:
Give the trait method a failure channel: ensure_plugin_state_any now
returns Option<&mut (dyn Any + Send + Sync)>, mirroring the sibling
plugin_state_any_mut on the same implementations, and the typed helper
returns Option<&mut T> (any?.downcast_mut). The panicking
implementations (ipc v3, ipc v4, NullHost, and the three never-called
test dummies) return None; the in-process app host wraps its real
storage in Some with unchanged insert-or-return semantics. The init
closure is now guarded by take() with a unit-Any fallback, so even an
implementation invoking it twice fails the typed downcast instead of
panicking. Object safety, argument types and vtable slot are
unchanged; external plugin sources get a compile error at the helper
and adopt Option handling on rebuild. Regression tests use catch_unwind
so they compile against both signatures: one per panic site (out-of-
process, NullHost, type mismatch), each additionally asserting the
None result and that pre-existing state survived.
This commit is contained in:
1 parent
c95d42a796
commit
b1f4047f9c
7 files changed
+89
-26
No files matched your search
@@ -833,11 +833,12 @@ pub trait HostApi {
|
||||
fn plugin_state_any(&self, plugin_id: &str) -> Option<&(dyn Any + Send + Sync)>;
|
||||
fn plugin_state_any_mut(&mut self, plugin_id: &str) -> Option<&mut (dyn Any + Send + Sync)>;
|
||||
/// Get the state for `plugin_id`, inserting `init()`'s result if absent.
|
||||
/// Returns `None` when this host cannot store state (e.g. out-of-process).
|
||||
fn ensure_plugin_state_any(
|
||||
&mut self,
|
||||
plugin_id: &'static str,
|
||||
init: &mut dyn FnMut() -> Box<dyn Any + Send + Sync>,
|
||||
) -> &mut (dyn Any + Send + Sync);
|
||||
) -> Option<&mut (dyn Any + Send + Sync)>;
|
||||
|
||||
// ── DocumentReader (added in API v3; appended at the end to keep vtable
|
||||
// indices stable for API v2 plugins) ─────────────────────────────────────
|
||||
@@ -1159,17 +1160,20 @@ pub fn plugin_state_mut<'a, T: Any + Send + Sync>(
|
||||
}
|
||||
|
||||
/// Typed get-or-insert of per-tab plugin state stored under `plugin_id`.
|
||||
///
|
||||
/// Returns `None` when this host cannot hold state (out-of-process plugins)
|
||||
/// or when existing state under `plugin_id` has a different type.
|
||||
pub fn ensure_plugin_state<'a, T: Any + Send + Sync>(
|
||||
host: &'a mut dyn HostApi,
|
||||
plugin_id: &'static str,
|
||||
init: impl FnOnce() -> T,
|
||||
) -> &'a mut T {
|
||||
) -> Option<&'a mut T> {
|
||||
let mut init = Some(init);
|
||||
let any = host.ensure_plugin_state_any(plugin_id, &mut || {
|
||||
Box::new((init.take().expect("init called once"))())
|
||||
let any = host.ensure_plugin_state_any(plugin_id, &mut || match init.take() {
|
||||
Some(make) => Box::new(make()),
|
||||
None => Box::new(()),
|
||||
});
|
||||
any.downcast_mut::<T>()
|
||||
.expect("plugin state type mismatch for plugin_id")
|
||||
any?.downcast_mut::<T>()
|
||||
}
|
||||
|
||||
#[cfg(feature = "host")]
|
||||
|
||||
@@ -364,10 +364,11 @@ impl HostApi for PluginHostApi {
|
||||
&mut self,
|
||||
_plugin_id: &'static str,
|
||||
_init: &mut dyn FnMut() -> Box<dyn Any + Send + Sync>,
|
||||
) -> &mut (dyn Any + Send + Sync) {
|
||||
) -> Option<&mut (dyn Any + Send + Sync)> {
|
||||
// Same limitation as `plugin_state_any`. This would need a serializable
|
||||
// state contract to work across processes.
|
||||
panic!("ensure_plugin_state is not supported for out-of-process plugins; keep state in the plugin crate")
|
||||
// state contract to work across processes. Degrade to `None` so plugins
|
||||
// keep running with state unavailable instead of crashing the runner.
|
||||
None
|
||||
}
|
||||
|
||||
fn document_reader(&self) -> Box<dyn DocumentReader + '_> {
|
||||
@@ -633,6 +634,22 @@ mod tests {
|
||||
(api, client_stream)
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn ensure_plugin_state_does_not_panic_for_out_of_process_hosts() {
|
||||
let (mut api, _peer) = make_client();
|
||||
let outcome = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
|
||||
let _ = api.ensure_plugin_state_any("opencad.demo", &mut || Box::new(7u32));
|
||||
}));
|
||||
assert!(
|
||||
outcome.is_ok(),
|
||||
"out-of-process ensure_plugin_state must degrade, not panic: {:?}",
|
||||
outcome.err()
|
||||
);
|
||||
assert!(api
|
||||
.ensure_plugin_state_any("opencad.demo", &mut || Box::new(7u32))
|
||||
.is_none());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn push_info_emits_request() {
|
||||
let (mut api, mut peer) = make_client();
|
||||
|
||||
@@ -607,8 +607,10 @@ impl HostApi for V4PluginHostApi {
|
||||
&mut self,
|
||||
_plugin_id: &'static str,
|
||||
_init: &mut dyn FnMut() -> Box<dyn Any + Send + Sync>,
|
||||
) -> &mut (dyn Any + Send + Sync) {
|
||||
panic!("ensure_plugin_state is not supported for out-of-process plugins; keep state in the plugin crate")
|
||||
) -> Option<&mut (dyn Any + Send + Sync)> {
|
||||
// Same limitation as `plugin_state_any`; degrade to `None` so the
|
||||
// plugin keeps running instead of dying on its own helper call.
|
||||
None
|
||||
}
|
||||
|
||||
fn document_reader(&self) -> Box<dyn DocumentReader + '_> {
|
||||
|
||||
@@ -128,8 +128,8 @@ impl HostApi for NullHost {
|
||||
&mut self,
|
||||
_plugin_id: &'static str,
|
||||
_init: &mut dyn FnMut() -> Box<dyn std::any::Any + Send + Sync>,
|
||||
) -> &mut (dyn std::any::Any + Send + Sync) {
|
||||
panic!("NullHost: ensure_plugin_state_any not available")
|
||||
) -> Option<&mut (dyn std::any::Any + Send + Sync)> {
|
||||
None
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1158,6 +1158,22 @@ mod tests {
|
||||
let runner = distinct_runner_path(&host);
|
||||
assert_eq!(runner, PathBuf::from("/app/OpenCADStudio-plugin-runner"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn null_host_ensure_plugin_state_does_not_panic() {
|
||||
let mut null = NullHost;
|
||||
let outcome = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
|
||||
let _ = null.ensure_plugin_state_any("opencad.demo", &mut || Box::new(7u32));
|
||||
}));
|
||||
assert!(
|
||||
outcome.is_ok(),
|
||||
"NullHost ensure_plugin_state must degrade, not panic: {:?}",
|
||||
outcome.err()
|
||||
);
|
||||
assert!(null
|
||||
.ensure_plugin_state_any("opencad.demo", &mut || Box::new(7u32))
|
||||
.is_none());
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(all(test, feature = "host"))]
|
||||
@@ -1261,8 +1277,8 @@ mod timeout_tests {
|
||||
&mut self,
|
||||
_plugin_id: &'static str,
|
||||
_init: &mut dyn FnMut() -> Box<dyn std::any::Any + Send + Sync>,
|
||||
) -> &mut (dyn std::any::Any + Send + Sync) {
|
||||
panic!("not used")
|
||||
) -> Option<&mut (dyn std::any::Any + Send + Sync)> {
|
||||
None
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -351,8 +351,8 @@ mod tests {
|
||||
&mut self,
|
||||
_plugin_id: &'static str,
|
||||
_init: &mut dyn FnMut() -> Box<dyn std::any::Any + Send + Sync>,
|
||||
) -> &mut (dyn std::any::Any + Send + Sync) {
|
||||
panic!("not used")
|
||||
) -> Option<&mut (dyn std::any::Any + Send + Sync)> {
|
||||
None
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -468,8 +468,8 @@ mod tests {
|
||||
&mut self,
|
||||
_plugin_id: &'static str,
|
||||
_init: &mut dyn FnMut() -> Box<dyn std::any::Any + Send + Sync>,
|
||||
) -> &mut (dyn std::any::Any + Send + Sync) {
|
||||
panic!("not used")
|
||||
) -> Option<&mut (dyn std::any::Any + Send + Sync)> {
|
||||
None
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
+31
-7
@@ -2612,12 +2612,14 @@ impl HostApi for HostSession<'_> {
|
||||
&mut self,
|
||||
plugin_id: &'static str,
|
||||
init: &mut dyn FnMut() -> Box<dyn Any + Send + Sync>,
|
||||
) -> &mut (dyn Any + Send + Sync) {
|
||||
self.app.tabs[self.tab]
|
||||
.plugin_state
|
||||
.entry(plugin_id)
|
||||
.or_insert_with(|| init())
|
||||
.as_mut()
|
||||
) -> Option<&mut (dyn Any + Send + Sync)> {
|
||||
Some(
|
||||
self.app.tabs[self.tab]
|
||||
.plugin_state
|
||||
.entry(plugin_id)
|
||||
.or_insert_with(|| init())
|
||||
.as_mut(),
|
||||
)
|
||||
}
|
||||
fn document_reader(&self) -> Box<dyn ocs_plugin_api::host::DocumentReader + '_> {
|
||||
Box::new(ocs_plugin_api::host::CadDocumentReader(self.document()))
|
||||
@@ -9157,7 +9159,7 @@ step('undo_move', lambda: M.move([u], (0, 0, 0), (7, 0, 0)))
|
||||
// Absent before first use.
|
||||
assert!(host::plugin_state::<u32>(&*host, "opencad.demo").is_none());
|
||||
// Insert via ensure, then mutate.
|
||||
*host::ensure_plugin_state(host, "opencad.demo", || 7u32) += 1;
|
||||
*host::ensure_plugin_state(host, "opencad.demo", || 7u32).unwrap() += 1;
|
||||
assert_eq!(
|
||||
*host::plugin_state::<u32>(&*host, "opencad.demo").unwrap(),
|
||||
8
|
||||
@@ -9169,6 +9171,28 @@ step('undo_move', lambda: M.move([u], (0, 0, 0), (7, 0, 0)))
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn ensure_plugin_state_type_mismatch_does_not_panic() {
|
||||
use ocs_plugin_api::host::{self, HostApi};
|
||||
let mut app = OpenCADStudio::new_for_test();
|
||||
let mut session = HostSession::new(&mut app, 0);
|
||||
let host: &mut dyn HostApi = &mut session;
|
||||
let _ = host::ensure_plugin_state(host, "opencad.demo", || 7u32);
|
||||
let outcome = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
|
||||
let _ =
|
||||
host::ensure_plugin_state::<String>(&mut *host, "opencad.demo", || "x".to_string());
|
||||
}));
|
||||
assert!(
|
||||
outcome.is_ok(),
|
||||
"type mismatch in ensure_plugin_state must degrade, not panic: {:?}",
|
||||
outcome.err()
|
||||
);
|
||||
assert!(
|
||||
host::ensure_plugin_state::<String>(host, "opencad.demo", || "x".to_string()).is_none()
|
||||
);
|
||||
assert_eq!(*host::ensure_plugin_state::<u32>(host, "opencad.demo", || 0).unwrap(), 7);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn add_entities_batch_assigns_handles_and_publishes_once() {
|
||||
let mut app = OpenCADStudio::new_for_test();
|
||||
|
||||
Reference in new issue
Block a user