Allow disconnection of type-safe signals #1198

andersgaustad · 2025-06-10T14:11:24Z

Would possibly solve #1113

Connecting typed signals with object.signals().signalname().connect_*() and object.signals().signalname().builder().connect_*() now returns a ConnectHandle that can be used to disconnect the signal and receiver once it is no longer needed.

#[derive(GodotClass)]
#[class(init, base=Node)]
struct SignalNode {
    #[var]
    #[init(val = 0i32)]
    counter: i32,

    _base: Base<Node>,
}

#[godot_api]
impl SignalNode {
    #[signal]
    fn notify();

    fn increment_self(&mut self) {
        self.counter += 1;
    }
}

fn disconnect_self(ctx: &TestContext) {
    let mut tree = ctx.scene_tree.clone();

    let mut signal_node = SignalNode::new_alloc();
    tree.add_child(&signal_node);

    signal_node.bind_mut().set_counter(0);
    let connection_handle = signal_node
        .signals()
        .notify()
        .connect_self(SignalNode::increment_self);

    signal_node.signals().notify().emit();

    assert_eq!(signal_node.bind().get_counter(), 1i32);

    // Disconnect signal
    connection_handle.disconnect();
}

It is not mandatory to save the returned ConnectHandle, but if it is kept around it can be used to disconnect any connections to that object before it dies.

For example:

#[derive(GodotClass)]
#[class(init, base=Node)]
struct DisconnectOnDropNode {
    connections_to_me: Vec<ConnectHandle>,

    _base: Base<Node>,
}

impl Drop for DisconnectOnDropNode {
    fn drop(&mut self) {
        for connection_handle in self.connections_to_me.drain(0..) {
            connection_handle.disconnect();
        }
    }
}

... should remove any connections from a source to DisconnectOnDropNode when it is dropped if the handles are stored in connections_to_me. This must be done manually however as there is no "detection" of signals targeting "dead" receivers.

Yarwin · 2025-06-10T14:23:51Z

Thanks!
Please, rebase on current master (i.e. history should be something along the lines of history of master -> your changes on top of it, without any merge commit and whatnot) – it is hard to review the changes otherwise 😅. The easiest way to do so would be something along the lines of:

# Make sure master is up-to-date
git reset --soft master
git commit -am "..."
# Will rewrite history, so tread carefully 
git push -f

(Personally I'm cool with ConnectHandle presented in description, sounds good)

GodotRust · 2025-06-10T14:55:44Z

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1198

andersgaustad · 2025-06-10T15:06:50Z

Thanks! Please, rebase on current master (i.e. history should be something along the lines of history of master -> your changes on top of it, without any merge commit and whatnot) – it is hard to review the changes otherwise 😅. The easiest way to do so would be something along the lines of:
# Make sure master is up-to-date
git reset --soft master
git commit -am "..."
# Will rewrite history, so tread carefully 
git push -f
(Personally I'm cool with ConnectHandle presented in description, sounds good)

Woops, sorry about that.
Should be fixed now

Bromeon

Thank you, this is a great addition! 👍

Thanks also for the thoroughly testing. However, I feel like 400 LoC to test disconnection is a bit much. While it's worthwhile to have good test coverage, we also need to keep in mind:

All code needs to be maintained, including tests.
Every #[itest] and registered #[derive(GodotClass)] increases the time it takes to run all integration tests.

The effects are small, but they add up, so we try to keep things at a balance 🙂

Some ideas where I see potential to reduce the amount of test code:

Extract common code patterns (just refactoring, I made a comment inline).
Do not retest other interactions that are known to work
- Example of this is DisconnectOnDropNode. We test elsewhere that Drop is invoked when a node is freed, and we know that Rust Drop generally works. So I don't really see the added value over a simpler procedural test that explicitly destroys connections at the call site. This would also result in simpler flow.
- Same with having 10 nodes and then doing modulo to pick some. Just declare two nodes, one with and one without the feature to test.
Could you combine disconnect_other_builder and disconnect_self_builder into one? I feel like the handler could just ignore one of the nodes, no?

Maybe you have other ideas? 🙂

Bromeon · 2025-06-10T14:53:02Z

godot-core/src/registry/signal/connect_handle.rs

+    /// Disconnects the signal from the connected callable.
+    ///
+    /// # Panics
+    /// If the connection does not exists.


Suggested change

/// If the connection does not exists.

/// If the connection does not exist.

godot-core/src/registry/signal/connect_handle.rs

itest/rust/src/builtin_tests/containers/signal_test.rs

Bromeon · 2025-06-10T15:28:48Z

itest/rust/src/builtin_tests/containers/signal_test.rs

+        let mut signal_node = SignalNode::new_alloc();
+        tree.add_child(&signal_node);


I don't think you need to add the node to the tree here, do you? This was done in some tests because Godot's own lifecycle signals were tested. Here, you have a custom signal notify however.

By the way, can you rename that signal to something like my_signal? The name notify is unfortunate as it looks a bit like Object::notify and the whole notification mechanism.

Bromeon · 2025-06-10T15:32:49Z

itest/rust/src/builtin_tests/containers/signal_test.rs

+            .get(0)
+            .unwrap();


Suggested change

.get(0)

.unwrap();

.at(0);

andersgaustad · 2025-06-10T21:36:57Z

Thank you - will take a look at this and adapt the code.

Thanks also for the thoroughly testing. However, I feel like 400 LoC to test disconnection is a bit much

Absolutely - idea was to ensure that every variation was working in every circumstance, but in hindsight I can see that it probably became much larger than it needs to be. Will try to merge the tests and remove any redundant checks.

Should I make a new commit for the new changes or squash them with this one?

Bromeon · 2025-06-10T22:04:01Z

You can squash the commits 🙂

(Force-pushes still show up in GitHub PRs, so one can still inspect the diff).

Yarwin · 2025-06-11T06:37:39Z

godot-core/src/registry/signal/typed_signal.rs

+        &self,
+        callable: &Callable,
+        flags: Option<ConnectFlags>,
+    ) -> (Gd<Object>, &str) {


Silly question, but can't we just return ConnectHandle here? CC: @Bromeon

In all the cases we use returned values only to create the ConnectHandle (ConnectHandle::new(owned_object, callable, signal_name.into())). Function signatures already tell what is being returned, and we are not going to use these values in any other way. (i.e. – it makes code a little more noisy and repetitive for no benefit, which is no biggie tbh)

(Not a fan of Into by the way 😅. I was pretty sure it was a StringName! Ok, that's a bigger offender)

Makes sense!

Also agree with into(), you could write this instead -- combined with the other change from GString to StringName:

StringName::from(signal_name)

andersgaustad · 2025-06-11T16:46:32Z

Made some changes - most notably refactoring the tests and making the pure connect() method returning a ConnectHandle as well. I removed the test that drops some random nodes that disconnect on death as it was a lot of code for a single test and because we do test that the disconnection really works in the other tests.

The handle now uses a Cow for the signal name. Also note that disconnect() no longer panics: I realized that the underlying obj.disconnect() does not actually panic but rather logs an error.

I also changed the signature of inner_connect_untyped to take ownership of the callable (instead of a reference) and return the ConnectHandle here instead.

Bromeon

Thanks for the refactors, great improvement! 200 LoC looks already much better 👍

One question, could you move the new tests to their own file signal_disconnect_test.rs? It would save one level of indentation, and you can also use an inner attribute #![cfg(...)].

Bromeon · 2025-06-12T10:46:46Z

godot-core/src/registry/signal/connect_handle.rs

+    /// Disconnects the signal from the connected callable.
+    ///
+    /// Generates an error if the connection does not exist.
+    /// Use [`is_connected()`][Self::is_connected] to make sure the connection exists.
+    pub fn disconnect(mut self) {
+        self.receiver_object
+            .disconnect(&*self.signal_name, &self.callable);
+    }


Godot errors are not that useful in Rust -- there's no way to programmatically handle them, and they can easily be overlooked. Since this is a higher-level API than Object.disconnect and Signal.disconnect, it would be good to make potential logic errors abundantly clear to the user.

We should probably panic in Debug mode:

Suggested change

/// Disconnects the signal from the connected callable.

///

/// Generates an error if the connection does not exist.

/// Use [`is_connected()`][Self::is_connected] to make sure the connection exists.

pub fn disconnect(mut self) {

self.receiver_object

.disconnect(&*self.signal_name, &self.callable);

}

/// Disconnects the signal from the connected callable.

///

/// # Panics (Debug)

/// If the signal is no longer connected. Use [`is_connected()`][Self::is_connected] to make sure the connection exists.

pub fn disconnect(mut self) {

debug_assert!(self.is_connected());

self.receiver_object

.disconnect(&*self.signal_name, &self.callable);

}

Maybe this deserves some discussion, how do you typically disconnect signals @andersgaustad @Yarwin @TitanNano? Are the connections usually valid beforehand?

As someone who has been eagerly awaiting type-safe signal disconnection, allow my two cents about this one.

In my current use-case, a single-player project of a smaller scale, every object is responsible for connecting to/disconnecting from the signals it needs based on game state/phase/whatnot changes, and in absolute majority of cases the connection is valid and if it's not, I probably messed something up.

That being said, I can see how in a bigger project where a lot of objects get constantly created/freed, keeping track of things might be more problematic. I was wondering if type-safe signal disconnection would include any sort of safety checks or if users would have to do it explicitly.

Thanks for the input. So my suggested approach with panics would indeed reveal logic errors in your case, and shouldn't happen in exported builds. (I'm also tending to have fewer panics in Release, because they are usually quite player-hostile, and a no-op disconnect isn't fatal)...

Note that it's always possible to use

if handle.is_connected() { handle.disconnect(); }

as a pattern for fallible disconnect.

Good idea.

expect_panic("disconnect invalid handle", || { connect_handle.disconnect(); });

Should maybe restore this test as well.
Do we expect to always run the tests in Debug?
If not, should we surround the expect_panic in a if cfg!(debug_assertions) { ... } or something?

There is expect_debug_panic_or_release_ok for this purpose 🙂

Connections have to have been valid at some point, but this is already guaranteed by the presence of the ConnnectHandle so I wouldn't touch the Godot behavior. If someone disconnects the same signal twice, they'll get an error log from the engine, but otherwise it should be fine. I don't see why we should replace that with our own panic.

I don't see why we should replace that with our own panic.

Like @brezhnevtusks mentioned: it immediately reveals logic errors.

Godot is known to be spammy with errors + warnings, and unfortunately some of them are meaningless or not actionable. Just look at our dodge-the-creeps CI, I don't remember a time when there were zero UID warnings on all platforms, and I've updated UIDs multiple times. Probably some tiny misconfiguration, but it works anyway. Point being, this has unfortunately led to a culture where errors are often ignored, plus it's easy to miss them when not looking at the console. It's not just possible, but extremely likely to miss them in CI.

In godot-rust, generated class APIs generally work with printed errors (because it would be monumental to translate all to panics, and often not desired), but hand-written APIs almost always use Result (if recoverable) or panic (if logic error). For builtin types, I went the extra mile to make error handling idiomatic in Rust: 🙂

Packed*Array::encode_*, decode_*, compress

Rect2::intersect

GString::find

_{(The reason why panic and not Result is that double-disconnect is a logic error, and there's no good way to handle it at runtime. People who still want to not care can check is_connected() first, just like they could precede Gd::free() with is_valid_instance())}

TypedSignal + ConnectHandle being high-level "type-safe" APIs, I don't see why users shouldn't be informed about mistakes? I would suggest we start with a panic and see how it's used -- it's always possible to relax, but it's much harder to introduce a panic into an API that didn't panic before.

Are the connections usually valid beforehand?

In my case they are always valid upon disconnection (similarly to is_instance_valid – it is better to address the root issue).

Bromeon · 2025-06-12T10:48:47Z

godot-core/src/registry/signal/connect_handle.rs

+    // Note: Like Godot's [`disconnect()`][https://docs.godotengine.org/en/stable/classes/class_object.html#class-object-method-disconnect]
+    // and [`Signal::disconnect`][crate::builtin::Signal::disconnect] this raises an error, but *does not panic*.


Regular // comments are not Markdown, so this link formatting is obstructing readability.

But with the previous change, this can probably be removed.

Absolutely - this used to be part of the docs before I turned it into a regular comment.
But I agree that just removing it is the correct choice now.

Bromeon · 2025-06-12T10:56:49Z

godot-core/src/registry/signal/connect_handle.rs

+    /// Returns `true` if the handle represents a valid connection
+    ///
+    /// Connections managed by a handle should normally always be valid, and in these cases this will always return `true`.
+    ///
+    /// However, if the signals and callables managed by this handle have been disconnected in any other way than by using
+    /// [`disconnect()`][Self::disconnect] -- e.g., using [`signal.disconnect()`][crate::builtin::Signal::disconnect] or
+    /// [`object.disconnect()`][crate::classes::Object::disconnect] -- this will return `false` instead.
+    pub fn is_connected(&self) -> bool {


I don't think the sentence

Connections managed by a handle should normally always be valid, and in these cases this will always return true.

conveys any useful information. People specifically call this method if they're interested when the connection is not valid, and "normally" doesn't help either. Let's keep docs as concise and information-dense as possible 🙂

Suggested change

/// Returns `true` if the handle represents a valid connection

///

/// Connections managed by a handle should normally always be valid, and in these cases this will always return `true`.

///

/// However, if the signals and callables managed by this handle have been disconnected in any other way than by using

/// [`disconnect()`][Self::disconnect] -- e.g., using [`signal.disconnect()`][crate::builtin::Signal::disconnect] or

/// [`object.disconnect()`][crate::classes::Object::disconnect] -- this will return `false` instead.

pub fn is_connected(&self) -> bool {

/// Whether the handle represents a valid connection.

///

/// Returns false if the object is no longer alive.

///

/// Returns _also_ false if the signals and callables managed by this handle have been disconnected in any other way than by using

/// [`disconnect()`][Self::disconnect] -- e.g. through [`Signal::disconnect()`][crate::builtin::Signal::disconnect] or

/// [`Object::disconnect()`][crate::classes::Object::disconnect].

pub fn is_connected(&self) -> bool {

This should maybe be tested, what do you think? 🤔

is_connected() after calling free() on the object

is_connected() after calling Object.disconnect() on the object

Sure, this is much more concise.

is_connected() after calling free() on the object

Good idea, will add that.
What do we expect when calling is_connected() on a freed object? A panic or false? I lean towards false, but open to both.

is_connected() after calling Object.disconnect() on the object

Do you mean testing is_connected() panics if we do obj.disconnect(signal, callable)?
We already test signal.disconnect(callable) before is_connected(), but I don't mind adding one for the object variant 🙂

What do we expect when calling is_connected() on a freed object? A panic or false? I lean towards false, but open to both.

Now that I think about it, this may actually not be that easy -- we don't actively store an object, since we use custom callables. It might be possible to associate one in the case of connect_self/connect_other calls.

But for now, maybe skip the is_connected()-after-object-freed test and write a // TODO: comment in the test. We can then look into it after this PR 🙂

Will do 👍

Just to make sure we agree: the broadcasting object is stored in the ConnectHandle, so we might be able to do something for the case of:

let obj = SignalObject::new_alloc(); let handle = obj.signals().my_signal().connect(...); obj.free(); handle.disconnect(); // <-- This will try to disconnect from a freed object

It could be possible to check if the object has been freed / is valid with is_instance_valid() (and return false if it is not), but the wording of the documentation seems to discourage it.

Intuitively, I would say:

Disconnecting a handle that's pointing to a freed object is a mistake and should panic.

Just like an already disconnected signal prevents further disconnection.

Checking connection (is_connected) on a handle pointing to a dead object should return false.

API-wise, there is no other way to verify in ConnectHandle whether it's safe to disconnect.

I think it's a feature of Godot signals to disconnect when objects die? Not 100% sure though 🤔

Yes, agree with your points 👍

I think it's a feature of Godot signals to disconnect when objects die? Not 100% sure though 🤔

If the signal object dies, yes. If the object behind the callable dies, not.

Bromeon · 2025-06-12T10:58:13Z

itest/rust/src/builtin_tests/containers/signal_test.rs

+mod connection_handles {
+    use crate::builtin_tests::containers::signal_test::connection_handles;
+    use crate::framework::itest;
+    use godot::{prelude::*, register::ConnectHandle};


Please flatten imports, and avoid prelude if possible.
You can however write use super::* to avoid duplicating all the parent symbols.

Bromeon · 2025-06-12T10:59:04Z

itest/rust/src/builtin_tests/containers/signal_test.rs

+        #[var]
+        #[init(val = 0i32)]
+        counter: i32,


Question: is #[var] necessary for the test here?

We can just access it as signal_object.bind().counter, no?

Bromeon · 2025-06-12T11:02:31Z

itest/rust/src/builtin_tests/containers/signal_test.rs

+        let signal_object = SignalObject::new_alloc();
+        let moved_clone = signal_object.clone();
+        let connection_handle = signal_object.signals().my_signal().connect(move || {
+            let mut my_signal_object = moved_clone.clone();
+            my_signal_object.bind_mut().increment_self();
+        });


I don't think cloning twice is necessary?

Also maybe use shorter identifiers -- this gives more weight to the other symbols like invoked methods:

Suggested change

let signal_object = SignalObject::new_alloc();

let moved_clone = signal_object.clone();

let connection_handle = signal_object.signals().my_signal().connect(move || {

let mut my_signal_object = moved_clone.clone();

my_signal_object.bind_mut().increment_self();

});

let obj = SignalObject::new_alloc();

let mut obj_moved = obj.clone();

let handle = obj.signals().my_signal().connect(move || {

obj_moved.bind_mut().increment_self();

});

Bromeon · 2025-06-12T11:03:45Z

itest/rust/src/builtin_tests/containers/signal_test.rs

+        eprintln!(
+            "Connected: {:?}",
+            godot_signal.is_connected(&godot_callable)
+        );


Debug statement still necessary?

Good catch, this was not supposed to be here...

Bromeon · 2025-06-12T11:05:51Z

itest/rust/src/builtin_tests/containers/signal_test.rs

+        godot_signal.disconnect(&godot_callable);
+
+        // If we check the connection, we should be able to see that connection is invalid and avoid a panic.
+        let is_valid = connect_handle.is_connected();
+        assert!(!is_valid);


Comment is not necessary. It's obvious that is_connected should be false after disconnect, and mention of the panic is not really relevant here.

Suggested change

godot_signal.disconnect(&godot_callable);

// If we check the connection, we should be able to see that connection is invalid and avoid a panic.

let is_valid = connect_handle.is_connected();

assert!(!is_valid);

obj.disconnect(&godot_callable);

assert!(!obj.is_connected());

Bromeon · 2025-06-12T11:06:20Z

itest/rust/src/builtin_tests/containers/signal_test.rs

+        let signal_object = SignalObject::new_alloc();
+
+        let connect_handle = signal_object


Also here and in other places, you could simply name them obj and handle 🙂

andersgaustad · 2025-06-12T11:47:39Z

Thanks - will try to push a new version soon.

One question, could you move the new tests to their own file signal_disconnect_test.rs? It would save one level of indentation, and you can also use an inner attribute #![cfg(...)].

Sure - where should me put the new file? Under itest/rust/src/builtin_tests/containers/ as a sibling to signal_test.rs?

Bromeon · 2025-06-12T11:49:04Z

Yes, sounds good! Might need one or the other pub(super) in signal_test.rs 🙂

Changes some of the `connect_` methods for typed signals to return a handle containing information about the connection. Can be used to disconnect the signal when not needed. Makes it simpler to disconnect signals to objects that are dead / about to die: Object can call disconnect on handles before being freed (for example in a "impl Drop" implementation).

andersgaustad · 2025-06-12T14:59:35Z

Pushed a new version.

Largest change to the ConnectHandle is arguably that disconnect() now panics in Debug. I think this makes sense as disconnecting the same connection twice is probably something you never intend to do, so if the connection for some reason is not valid once we disconnect then I feel it makes sense for the ConnectHandle to loudly announce so.
That being said, the alternative would be that Godot raises an error instead which also would inform any user of the faulty disconnect and I think that would also be fine (but I prefer this as the panic is raised by Rust instead of Godot).

Also moved the tests to signal_disconnect_test.rs, and did some minor changes (e.g., testing now handle statuses after both signal.disconnect(callable) and object.disconnect(signal_name, callable).

Bromeon

Thanks a lot for the changes! Looks good, some tiny formatting parts that I can address later.

Btw, since the topic came up on Discord: I know the bar is sometimes a bit high for code style, and some comments are definitely nitpicks, so let me know if it gets too much. One option is that I modify the PR directly by pushing to it, but I don't want to do this without checking back first 🙂

andersgaustad · 2025-06-12T22:58:20Z

Great to hear 🙂
Feel free to push directly - I don't mind at all

Bromeon added feature Adds functionality to the library c: core Core components labels Jun 10, 2025

Bromeon linked an issue Jun 10, 2025 that may be closed by this pull request

Support disconnection for type-safe signals #1113

Open

andersgaustad force-pushed the disconnect-typed-signal branch from 53f6ff9 to 20084ba Compare June 10, 2025 15:03

Bromeon reviewed Jun 10, 2025

View reviewed changes

Yarwin reviewed Jun 11, 2025

View reviewed changes

andersgaustad force-pushed the disconnect-typed-signal branch from 20084ba to 235c226 Compare June 11, 2025 16:38

Bromeon reviewed Jun 12, 2025

View reviewed changes

andersgaustad force-pushed the disconnect-typed-signal branch from 235c226 to a6580fb Compare June 12, 2025 14:43

Bromeon approved these changes Jun 12, 2025

View reviewed changes

	/// If the connection does not exists.
	/// If the connection does not exist.

		let mut signal_node = SignalNode::new_alloc();
		tree.add_child(&signal_node);

		// Note: Like Godot's [`disconnect()`][https://docs.godotengine.org/en/stable/classes/class_object.html#class-object-method-disconnect]
		// and [`Signal::disconnect`][crate::builtin::Signal::disconnect] this raises an error, but does not panic.

		let signal_object = SignalObject::new_alloc();

		let connect_handle = signal_object

Uh oh!

Allow disconnection of type-safe signals #1198

Are you sure you want to change the base?

Allow disconnection of type-safe signals #1198

Uh oh!

Conversation

andersgaustad commented Jun 10, 2025

Uh oh!

Yarwin commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GodotRust commented Jun 10, 2025

Uh oh!

andersgaustad commented Jun 10, 2025

Uh oh!

Bromeon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andersgaustad commented Jun 10, 2025

Uh oh!

Bromeon commented Jun 10, 2025

Uh oh!

Yarwin Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andersgaustad commented Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bromeon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bromeon Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Yarwin commented Jun 10, 2025 •

edited

Loading

Yarwin Jun 11, 2025 •

edited

Loading

andersgaustad commented Jun 11, 2025 •

edited

Loading

Bromeon Jun 12, 2025 •

edited

Loading