Skip to content

Commit c730734

Browse files
committed
Fixed bug causing ConnectHandle::is_connected() to sometimes panic.
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()`.
1 parent e1073bc commit c730734

File tree

2 files changed

+48
-4
lines changed

2 files changed

+48
-4
lines changed

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,10 @@ impl ConnectHandle {
5454
/// [`disconnect()`][Self::disconnect] -- e.g. through [`Signal::disconnect()`][crate::builtin::Signal::disconnect] or
5555
/// [`Object::disconnect()`][crate::classes::Object::disconnect].
5656
pub fn is_connected(&self) -> bool {
57+
if !self.receiver_object.is_instance_valid() {
58+
return false;
59+
}
60+
5761
self.receiver_object
5862
.is_connected(&*self.signal_name, &self.callable)
5963
}

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

Lines changed: 44 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -152,10 +152,14 @@ fn handle_recognizes_direct_object_disconnect() {
152152
})
153153
}
154154

155-
#[itest(skip)]
156-
fn handle_recognizes_freed_object() {
157-
// TODO
158-
// We should define what happens when either the broadcasting object or (potential) receiving object is freed.
155+
#[itest]
156+
fn test_handle_after_freeing_broadcaster() {
157+
test_freed_nodes(true);
158+
}
159+
160+
#[itest]
161+
fn test_handle_after_freeing_receiver() {
162+
test_freed_nodes(false);
159163
}
160164

161165
// Helper functions:
@@ -229,6 +233,42 @@ fn test_handle_recognizes_non_valid_state(disconnect_function: impl FnOnce(&mut
229233
obj.free();
230234
}
231235

236+
fn test_freed_nodes(free_broadcaster_first: bool) {
237+
let broadcaster = SignalDisc::new_alloc();
238+
let receiver = SignalDisc::new_alloc();
239+
240+
let handle = broadcaster
241+
.signals()
242+
.my_signal()
243+
.connect_other(&receiver, |r| {
244+
r.increment_self();
245+
});
246+
247+
let (to_free, other) = if free_broadcaster_first {
248+
(broadcaster, receiver)
249+
} else {
250+
(receiver, broadcaster)
251+
};
252+
253+
// Free one of the nodes, and check if the handle thinks the objects are connected.
254+
// If the broadcaster is freed, its signals to other objects is implicitly freed as well. Thus, the handle should not be connected.
255+
// If the receiver is freed, the connection between the valid broadcaster and the non-valid freed object still exists.
256+
// In the latter case, the connection can - and probably should - be freed with disconnect().
257+
to_free.free();
258+
let is_connected = handle.is_connected();
259+
assert_ne!(
260+
free_broadcaster_first, is_connected,
261+
"Handle should only return false if broadcasting is freed!"
262+
);
263+
264+
// It should be possible to discconect a connection between a valid broadcaster and a non-valid receiver.
265+
if is_connected {
266+
handle.disconnect();
267+
}
268+
269+
other.free();
270+
}
271+
232272
fn has_connections(obj: &Gd<SignalDisc>) -> bool {
233273
!obj.get_signal_connection_list("my_signal").is_empty()
234274
}

0 commit comments

Comments
 (0)