Fixed bug causing ConnectHandle::is_connected() to sometimes panic.

[andersgaustad]

Follow-up to #1198

A ConnectHandle created from a connection from A to B would panic if is_connected() was called after A was freed. Fixed by is_connected() checking if the contained object is valid through is_instance_valid().

For example, the following will panic:

let broadcaster = SignalDisc::new_alloc();
let receiver = SignalDisc::new_alloc();
let handle = broadcaster
        .signals()
        .my_signal()
        .connect_other(&receiver, |r| {
            r.increment_self();
        });
        
broadcaster.free();
handle.is_connected(); // <--- Panics as handle's "receiver object" is freed!

(Note that if the receiver is freed the connection will still exist. Emitting the signal will then cause another panic, but disconnection works as intended. I feel personally this makes sense as the connection in this case does indeed exist, but that the connected receiver isn't valid, and that you normally want to disconnect in this case.)

The fix itself is very simple and achieved by using a if !self.receiver_object.is_instance_valid()-guard. As I think(?) we briefly discussed in #1198, the good aspect of this is that is_connected() (now) won't panic, but the documentation of is_instance_valid() seems to discourage its use.

EDIT: Removed part about disconnect() - this should always panic if A is freed.

[GodotRust]

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

[Bromeon]

A ConnectHandle created from a connection from A to B would panic if is_connected() or disconnect() was called after A was freed. Fixed by is_connected() checking if the contained object is valid through is_instance_valid().

Calling disconnect() on a non-connected signal would still panic, no?
You just change is_connected() in this PR, and thus the definition of "connected".

Because I think disconnecting an A->B connection after A has been freed is a logic error and should panic, see #1198 (comment).

[Bromeon]

If we go with this, could just use &&

[andersgaustad]

Because I think disconnecting an A->B connection after A has been freed is a logic error and should panic

That is a very good point. In addition to not being valid, A wouldn't have any signals either so trying to disconnect anything doesn't make sense.

I still feel is_connected() never panicking outweighs the "drawback" of using is_instance_valid(), but let me know if anyone disagrees. There might be other ways to have is_connected() check the validity of the broadcaster, but this was the simplest way I could think of.

[Yarwin]

I would suggest something along the lines of:

Suggested change

	self.receiver_object.is_instance_valid()
	if !self.receiver_object.is_instance_valid() {
	godot_warn!("...");
	return false
	}

1. This is something that probably comes from logic error which should be properly addressed in the userspace and hiding it from user kinda sucks 😅 (i.e. there is 0% chance that it will be spotted). I can't say for sure without analyzing solid usecases though 🤔.
2. We can always easily slap #[cfg(debug_assertions)] on it later. (or maybe never? or maybe now? Hell if I know, CC: @Bromeon)

[Yarwin]

hm, now I see the previous comment 🤔. I still personally think that we should emit some warning in such case though (correct me if I'm wrong ofc).

[Bromeon]

@andersgaustad could you tell us more about the use case / context in which you encountered this, and where you think is_connected() would behave better with the changes suggested in this PR? 🙂

[andersgaustad]

@andersgaustad could you tell us more about the use case / context in which you encountered this

Sure!

I have certain "Card" nodes that initializes certain connections when they are spawned. For all incoming connections (e.g., from the global singleton "GlobalEventBus"), I save the ConnectHandles in the following way:

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

    _base: Base<Node>,
}

I also implement Drop so that the connections are disconnected automatically once the cards are freed:

impl Drop for DemoCard {
    fn drop(&mut self) {
        for connection_handle in self.connections_to_me.drain(..) {
            if connection_handle.is_connected() {
                connection_handle.disconnect();
            }
        }
    }
}

This usually works fine, but I accidentally added a handle representing a connection for the card to itself. When the card tries to check the connection, is_connected() panics (silently) as it (assumedly) tries to check the saved non-valid broadcaster.
(Now that I think about it, I am actually a little unsure if this is what happens as it just crashes and closes the Play-in-Editor window 🤔 I can see that other panics typically raise an error in Godot)

In my case, this was something caused by a clearly unintended configuration, but it might be nice to prevent the panic nevertheless. That way you save and manage any number of handles, and check if the connections are valid regardless of if the underlying objects are freed or not.

On the other hand, I can also see that argument of "if you try to query about connections from a freed node you are most probably doing something wrong". I was able to easily work around the panic by simply not adding the handles for connections done from a node to itself. If that is what we end up with we could simply scrap the validity check and just change the documentation to state that is_connected() may panic?

[Yarwin]

I also implement Drop so that the connections are disconnected automatically once the cards are freed:

1. It is the best to handle it with NOTIFICATION_PREDELETE https://docs.godotengine.org/en/stable/classes/class_object.html#class-object-private-method-notification
2. Godot should auto-magically disconnect all the related signals by itself when object is freed (including signals from object to itself) – if it is not doing so then we need to fix our implementation 😬. In other words:

extends Node
class_name MyLittleSignaller

signal my_signal

func _ready() -> void:
	await get_tree().create_timer(0.50).timeout
	print("connections to my signal: ", self.get_signal_connection_list("my_signal"))
	await get_tree().create_timer(1.50).timeout
	print("connections to my signal after Node2 has been pruned: ", self.get_signal_connection_list("my_signal"))
	my_signal.emit()
	queue_free()
(...)

extends Node
class_name Receiver

###############################################################################
# Builtin functions                                                           #
###############################################################################

func _ready() -> void:
	$"../Node".my_signal.connect(some_response)
	$"../Node".my_signal.connect(func(): print("anonymous lambda is not bound to any Object"))
	await get_tree().create_timer(1.0).timeout
	queue_free()
(…)

extends Node
class_name PatientListener

func _ready() -> void:
	$"../Node".my_signal.connect(receiver3)
	print("incoming connections at start: ", self.get_incoming_connections())
	await get_tree().create_timer(3.00).timeout
	print("incoming connections after Node has been pruned: ", self.get_incoming_connections())
	queue_free()

###############################################################################
# Public functions                                                            #
###############################################################################

func receiver3():
	print("hello from receiver!")

Prints:

incoming connections at start: [{ "signal": Node(node.gd)::[signal]my_signal, "callable": Node(node_3.gd)::receiver3, "flags": 0 }]
connections to my signal: [{ "signal": Node(node.gd)::[signal]my_signal, "callable": Node(node_2.gd)::some_response, "flags": 0 }, { "signal": Node(node.gd)::[signal]my_signal, "callable": <anonymous lambda>(lambda), "flags": 0 }, { "signal": Node(node.gd)::[signal]my_signal, "callable": Node(node_3.gd)::receiver3, "flags": 0 }]
connections to my signal after Node2 has been pruned: [{ "signal": Node(node.gd)::[signal]my_signal, "callable": <anonymous lambda>(lambda), "flags": 0 }, { "signal": Node(node.gd)::[signal]my_signal, "callable": Node(node_3.gd)::receiver3, "flags": 0 }]
anonymous lambda is not bound to any Object
hello from receiver!
incoming connections after Node has been pruned: []

And the behavior should be the same for Godot-rust! (if it doesn't behave the same we did a moderately-sized oopsie 😅 )

[andersgaustad]

It is the best to handle it with NOTIFICATION_PREDELETE

Oh, you are right! I was sure this was sent only for the node being freed but not for any of its children, but it seems like it works exactly as I had hoped. In hindsight I should probably have tested this before just assuming it worked that way 😅

Godot should auto-magically disconnect all the related signals by itself when object is freed (including signals from object to itself)

From what I understand, if we free A then all connections A -> B should be disconnected. If we free B instead, any A -> B connection is not disconnected. Emitting the signal in A will then lead to Godot logging the error mentioned in #1113 : {Class}::bind_mut: access to instance with ID {id} after it has been freed (though nothing else "breaks").
This is why I save the "GlobalEventBus -> Card"-ConnectHandles in my case, and disconnect them once the Cards are freed.

Just to be clear, if we have an "A -> B"-connection and free A then the connection should also be disconnected. However, if we store a ConnectHandle that has saved that connection and it tries to query if the connection "is connected" it will try to call .is_connected(signal_name, callable)on its saved copy of A and thus panic since A has been freed.

[Bromeon]

From what I understand, if we free A then all connections A -> B should be disconnected. If we free B instead, any A -> B connection is not disconnected.

Since we have the knowledge of B whenever one of the connect_self or connect_other methods is used, I wonder if auto-disconnect for B would be a useful feature? It would require us to track connections 🤔 but the alternative is likely that all users have to do this on their own...

[Yarwin]

From what I understand, if we free A then all connections A -> B should be disconnected.

I would also check if given Callables are pruned as well – i.e. if it doesn't result in some memory leak (I'm 95% sure that it does not? It won't hurt to check it anyway).

I wonder if auto-disconnect for B would be a useful feature?

Ideally it should work the same as in gdscript (i.e. all related signals are being disconnected when object is freed).

It would require us to track connections.

Either tracking or using Object::get_signal_connection_list/Object::get_incoming_connections 🤔.

[andersgaustad]

I would also check if given Callables are pruned as well – i.e. if it doesn't result in some memory leak

Did some quick testing, and:

let typed_a = SignalDisc::new_alloc();
let typed_b = SignalDisc::new_alloc();

typed_a.signals().my_signal().connect_other(
    &typed_b,
    |other| {
        other.increment_self();
    }
);

let signal_dict = typed_a.get_signal_connection_list("my_signal").at(0);
let signal = signal_dict.at("signal").to::<Signal>();
let callable = signal_dict.at("callable").to::<Callable>();

callable.call(&[]);
eprintln!("Typed: Before free: S: {:?} - C: {:?} - B: {}", &signal, &callable, typed_b.bind().counter);

typed_a.free();

callable.call(&[]);
eprintln!("Typed: After free: S: {:?} - C: {:?} - B: {}", &signal, &callable, typed_b.bind().counter);

typed_b.free();


let mut untyped_a = SignalDisc::new_alloc();
let untyped_b = SignalDisc::new_alloc();

let mut callable_clone = untyped_b.clone();
let inc_b_callable = Callable::from_local_fn(
    "inc_callable",
    move |_| {
        callable_clone.bind_mut().increment_self();
        Ok(Variant::nil())
    }
);
untyped_a.connect("my_signal", &inc_b_callable);

let signal_dict = untyped_a.get_signal_connection_list("my_signal").at(0);
let signal = signal_dict.at("signal").to::<Signal>();
let callable = signal_dict.at("callable").to::<Callable>();

callable.call(&[]);
eprintln!("Untyped: Before free: S: {:?} - C: {:?} - B: {}", &signal, &callable, untyped_b.bind().counter);

untyped_a.free();

callable.call(&[]);
eprintln!("Untyped: After free: S: {:?} - C: {:?} - B: {}", &signal, &callable, untyped_b.bind().counter);

untyped_b.free();

prints:

Typed: Before free: S: signal { name: &"my_signal", object: Some(Gd { id: 37144758086, class: SignalDisc }) } - C: Callable { method: None, object: None } - B: 1
Typed: After free: S: signal { name: &"my_signal", object: None } - C: Callable { method: None, object: None } - B: 2
Untyped: Before free: S: signal { name: &"my_signal", object: Some(Gd { id: 37178312519, class: SignalDisc }) } - C: Callable { method: None, object: None } - B: 1
Untyped: After free: S: signal { name: &"my_signal", object: None } - C: Callable { method: None, object: None } - B: 2

... which seems to indicate that typed and untyped signals behave the same when the broadcaster is freed?
I was a little surprised that the Callable works in both cases - i.e., still is valid and increments b - after a is freed, but the Callable is maybe freed once it goes out of scope?

Either tracking or using Object::get_signal_connection_list/Object::get_incoming_connections 🤔.

Something like:

let incoming = obj.get_incoming_connections();
for to_me in incoming.iter_shared() {
    let signal = to_me.at("signal").to::<Signal>();
    let callable = to_me.at("callable").to::<Callable>();
    signal.disconnect(&callable);
}

... though that would have to be done inside the "destructor" of Object when it recivies NOTIFICATION_PREDELETE, right?