Repository navigation
Fix panic-in-drop in future tests; harden against race conditions - #1617
Merged
Merged
Conversation
`FallibleSignalFuture::drop` unconditionally accessed the signal's object to disconnect the resolver callable. When the signal had already fired (the common case under `ONE_SHOT`), the connection was already gone, so the access was unnecessary -- and unsound when the object had been freed concurrently (e.g. a signal emitted from another thread that then drops the last reference). Touching a freed object inside `Drop` panics in `maybe_inc_ref` and escalates to a fatal non-unwinding abort. Now the drop only attempts disconnection while the future is still `Pending`; once `Ready`/`Dead`, it skips the object access entirely. Additionally, `Signal::object()` now honors its documented contract of returning `None` for dead objects instead of panicking, by validating instance liveness before incrementing the refcount.
The test deliberately drops the only reference to the signal's object on the worker thread, so the object is freed while the SignalFuture is still being dropped on the main thread. This is the exact scenario that previously aborted the process. Add a comment so the test is not later "fixed" by keeping the object alive, which would silently remove the regression coverage.
When the future is dropped before the signal fired, the still-live ONE_SHOT connection must be disconnected, which requires accessing the signal's object. That object can be freed concurrently -- reachable only when a `Gd` is moved across threads via the `unsafe` cross-thread contract (`Gd` is `!Send`) and freed there. The resulting access panics, and a panic escaping `Drop` aborts the process. Wrap the cleanup in `handle_panic` so such a race is contained: the stale connection is leaked (Godot removes it when the object dies) instead of aborting. The common case (object alive) disconnects normally. Update the `SignalFuture`/`FallibleSignalFuture` docs accordingly.
Bromeon
force-pushed
the
bugfix/signal-future-drop-freed-object
branch
from
May 23, 2026 18:17
f4544c5 to
f351fb2
Compare
|
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1617 |
TW0hank0
added a commit
to TW0hank0/positive_mahjong
that referenced
this pull request
Aug 19, 2026
Update patch dependencies This MR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [console_error_panic_hook](https://github.com/rustwasm/console_error_panic_hook) | workspace.dependencies | patch | `=0.1.5` β `=0.1.7` | | [godot](https://godot-rust.github.io) ([source](https://github.com/godot-rust/gdext)) | workspace.dependencies | patch | `=0.5.3` β `=0.5.5` | | [serde](https://serde.rs) ([source](https://github.com/serde-rs/serde)) | workspace.dependencies | patch | `=1.0.228` β `=1.0.229` | | [serde_json](https://github.com/serde-rs/json) | workspace.dependencies | patch | `=1.0.149` β `=1.0.151` | --- ### Release Notes <details> <summary>rustwasm/console_error_panic_hook (console_error_panic_hook)</summary> ### [`v0.1.6`](rustwasm/console_error_panic_hook@0.1.5...0.1.6) [Compare Source](rustwasm/console_error_panic_hook@0.1.5...0.1.6) </details> <details> <summary>godot-rust/gdext (godot)</summary> ### [`v0.5.5`](https://github.com/godot-rust/gdext/blob/HEAD/Changelog.md#v055) [Compare Source](godot-rust/gdext@v0.5.4...v0.5.5) *9 August 2026* ##### π» Features - `EngineBitfield`: add `with()` and `without()` methods ([#​1627](godot-rust/gdext#1627)) - `#[func(virtual)]` now supports `async fn` for GDScript coroutines ([#​1645](godot-rust/gdext#1645)) - Add `PhantomData<T>` support for `GodotConvert` derive macro ([#​1619](godot-rust/gdext#1619)) ##### π Performance - Singletons are now cached ([#​1636](godot-rust/gdext#1636)) - Cache user singleton pointers ([#​1644](godot-rust/gdext#1644)) - Cheaper refcount operations ([#​1669](godot-rust/gdext#1669)) ##### π§Ή Quality of life - Always use godot\_error in panic hook ([#​1647](godot-rust/gdext#1647)) - Rename `cast_int/cast_float` to `to_vector*/to_rect*` ([#​1661](godot-rust/gdext#1661)) - Validate class registrations against `ClassDB` ([#​1664](godot-rust/gdext#1664)) ##### π οΈ Bugfixes - Fix `#[var(pub)]` panic during `OnEditor<Gd<T>>` reload ([#​1639](godot-rust/gdext#1639)) - Fix async `SignalFuture` panic when object is freed before exit flag ([#​1648](godot-rust/gdext#1648)) - Fix `api-custom` build when prebuilt lags detected Godot version ([#​1649](godot-rust/gdext#1649)) - Harden various parts of codebase ([#​1660](godot-rust/gdext#1660)) - Fix `GFile` `BufRead` past EOF ([#​1670](godot-rust/gdext#1670)) ##### ποΈ Maintenance - Replace `godot-cell` borrow-state proptests with exhaustive model check ([#​1650](godot-rust/gdext#1650)) - Make accidental access of `#[doc(hidden)]` symbols more explicit ([#​1651](godot-rust/gdext#1651)) - Follow-up to rustc / Godot changes ([#​1659](godot-rust/gdext#1659)) - Name virtual-callback layers after user-facing `I*` virtuals ([#​1662](godot-rust/gdext#1662)) - Move ref-counting operations from `DynMemory` to `RawGd` ([#​1667](godot-rust/gdext#1667)) ### [`v0.5.4`](https://github.com/godot-rust/gdext/blob/HEAD/Changelog.md#v054) [Compare Source](godot-rust/gdext@v0.5.3...v0.5.4) *23 June 2026* ##### π» Features - Godot 4.7: add `api-4-7` level ([#​1641](godot-rust/gdext#1641)) - Type-Safe API for User-defined RPCs ([#​1535](godot-rust/gdext#1535), [#​1634](godot-rust/gdext#1634)) - String conversions: ` to_gstring()`, `to_string_name()` etc ([#​1623](godot-rust/gdext#1623)) - Thread-safe FFI scaffolding + cross-thread printing ([#​1632](godot-rust/gdext#1632)) - `Gd` introspection APIs: `dynamic_class`, `is_dynamic_class[_of]`, `is_ref_counted` ([#​1631](godot-rust/gdext#1631)) - Support async versions of `tools::load()` and `tools::try_load()` ([#​1601](godot-rust/gdext#1601)) - Support `@export_node_path` attribute ([#​1620](godot-rust/gdext#1620)) ##### π§Ή Quality of life - Slightly more flexible `GodotStringExt` through unsized impls ([#​1629](godot-rust/gdext#1629)) - Suppress Godot errors in fallible functions ([#​1637](godot-rust/gdext#1637)) ##### π οΈ Bugfixes - Fix Linux hot-reload panic when TLS destructors fire before init ([#​1611](godot-rust/gdext#1611)) - Fix panic-in-drop in future tests; harden against race conditions ([#​1617](godot-rust/gdext#1617)) - Auto-remove custom callables also when connected in untyped APIs ([#​1612](godot-rust/gdext#1612)) - Fix async panics at shutdown ([#​1625](godot-rust/gdext#1625)) - Fix `Object` returns with dynamic type `RefCounted` ([#​1628](godot-rust/gdext#1628)) - Fix memory leak for `connect_self()` on `RefCounted` ([#​1635](godot-rust/gdext#1635)) - Fix dangling singleton pointer after deinit ([#​1638](godot-rust/gdext#1638)) - Fix RPC borrow panic for `call_local` ([#​1643](godot-rust/gdext#1643)) ##### ποΈ Maintenance - Add `once` flag to `defer_startup_warn!` and `defer_startup_error!` ([#​1613](godot-rust/gdext#1613)) - CI: unzip occasionally fails; retry ([#​1616](godot-rust/gdext#1616)) - Rename self-registration parts: `plugin` -> `shard` ([#​1618](godot-rust/gdext#1618)) - Replace internal `PassiveGd` with safer `BorrowedGd` ([#​1633](godot-rust/gdext#1633)) ##### π Documentation - Document `register-docs` security; warn when used in export builds ([#​1615](godot-rust/gdext#1615)) - Document `#[class(tool)]` + `ready()` behavior across editor reload ([#​1614](godot-rust/gdext#1614)) - Fix borrow panic in `godot::task::spawn` doc example ([#​1622](godot-rust/gdext#1622)) </details> <details> <summary>serde-rs/serde (serde)</summary> ### [`v1.0.229`](https://github.com/serde-rs/serde/releases/tag/v1.0.229) [Compare Source](serde-rs/serde@v1.0.228...v1.0.229) - Update to syn 3 </details> <details> <summary>serde-rs/json (serde_json)</summary> ### [`v1.0.151`](https://github.com/serde-rs/json/releases/tag/v1.0.151) [Compare Source](serde-rs/json@v1.0.150...v1.0.151) - Add RawValue::from\_string\_unchecked ([#​1331](serde-rs/json#1331), thanks [@​WonderLawrence](https://github.com/WonderLawrence)) ### [`v1.0.150`](https://github.com/serde-rs/json/releases/tag/v1.0.150) [Compare Source](serde-rs/json@v1.0.149...v1.0.150) - Reject non-string enum object keys ([#​1324](serde-rs/json#1324), thanks [@​puneetdixit200](https://github.com/puneetdixit200)) </details> --- ### Configuration π **Schedule**: (in timezone Asia/Taipei) - Branch creation - At any time (no schedule defined) - Automerge - At any time (no schedule defined) π¦ **Automerge**: Disabled by config. Please merge this manually once you are satisfied. β» **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. π» **Immortal**: This MR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Mend Renovate CLI](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0NC4zMy4yIiwidXBkYXRlZEluVmVyIjoiNDQuMzMuMiIsInRhcmdldEJyYW5jaCI6Im1hc3RlciIsImxhYmVscyI6W119--> See merge request TW0hank0/positive_mahjong!142
This file contains hidden or 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
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.
In CI workflow runs A, B and C, job
godot-itest (linux-features-experimental)failed with a process abort (core dump). The Godot process printed:...but without any panic payload. It turned out to be a latent race condition in godot-rust; I could reproduce it on Godot version
fa6cad230. Running tests filtered by[async,signal_]on two cores (taskset -c 0,1) let the abort appear in ~1/76 runs.When a
SignalFutureis dropped,FallibleSignalFuture::dropdisconnects itsONE_SHOTresolver viaSignal::object(). If the signal's object is freed concurrently on another thread, that access panics -- and a panic insideDropbecomes a non-unwinding abort.This was only reachable through
unsafe, sinceGdis!Sendand must be bypassed to free it on another thread. In our case it's theThreadCrosser.This PR applies multiple fixes and hardenings:
droponly touches the object while the future is stillPending; once the signal fired, theONE_SHOTconnection is already gone. This eliminates the reproduced abort.Signal::object()not properly catching dead objects. We now useis_instance_valid()on the object returned by Godot's ownSignal::get_objectAPI and returnNonefor dead objects, instead of panicking (which could happen inDrop).Drop. For the rarePending-and-freed-concurrently TOCTOU race, cleanup is wrapped inhandle_panic. Instead of aborting, we skip thedisconnect()and leave the connection registered. Godot will disconnect all object's signal connections when the object is destroyed, releasing the reference it holds to our resolverCallable.