Support threads in the script debugger #76582

reduz · 2023-04-29T15:23:58Z

This implementation adds threads on the side of the client (script debugger).
Some functions of the debugger are optimized.
The profile is also now thread safe using atomics.
The editor has been upgraded to be able to edit multiple threads.

This PR adds threaded support for the script language debugger. Every thread has its own thread local data and it will connect to the debugger using multiple thread IDs. This means that, now, the editor can receive multiple threads entering debug mode at the same time.

Obligatory screenshot:

Fixes Breakpoints do not work in Thread #2446
Fixes Threads do not output errors to debugger #18330
Supersedes Add thread support to debugger #64364 (this aims to be a simpler implementation).

core/debugger/remote_debugger.cpp

editor/debugger/script_editor_debugger.cpp

core/debugger/remote_debugger.cpp

adamscott · 2023-04-30T13:21:25Z

I tried your PR. I paused my instance, while 2 threads were running in the background, but they didn't show up in the debugger. Is there something more to do?

I ran that code on my main scene:

extends Node

var thread_hello: Thread
var thread_world: Thread

func _ready() -> void:
	thread_hello = Thread.new()
	thread_world = Thread.new()

	thread_hello.start(_thread_func.bind("hello", 500), Thread.PRIORITY_NORMAL)
	thread_world.start(_thread_func.bind("world", 1000), Thread.PRIORITY_NORMAL)

func _thread_func(str: String, delay: int) -> void:
	while true:
		prints(str)
		OS.delay_msec(delay)

reduz · 2023-04-30T14:37:45Z

@adamscott Threads only show up in the debugger once they break with an error, otherwise they are not tracked. There is not much of a point to doing so, otherwise since the engine itself does not track the threads. If you want to know what they are doing, that will probably be possible with the threaded profiler.

Chaosus · 2023-05-01T06:12:16Z

Is it possible to assign a name for the Thread? Otherwise, it may be hard to detect which thread emit an error by only number.

Calinou

Remember to remove this line from the class reference:

godot/doc/classes/Thread.xml

Line 8 in ee86505

    
           [b]Note:[/b] Breakpoints won't break on code if it's running in a thread. This is a current limitation of the GDScript debugger.

vnen · 2023-05-02T18:33:10Z

Is it possible to assign a name for the Thread? Otherwise, it may be hard to detect which thread emit an error by only number.

Since we already have a Thread wrapper, it would be nice to add a name property to it. I agree that it helps debugging.

modules/gdscript/gdscript.cpp

vnen · 2023-05-02T19:10:26Z

Overall looks good to me. It is not that much of changes in the end. The editor side does need polish though, it is difficult to use as the pointer does not update correctly when it breaks from multiple threads.

editor/debugger/script_editor_debugger.cpp

reduz · 2023-05-04T10:49:46Z

Remember to remove this line from the class reference:

godot/doc/classes/Thread.xml

Line 8 in ee86505

[b]Note:[/b] Breakpoints won't break on code if it's running in a thread. This is a current limitation of the GDScript debugger.

Done

adamscott

Looks good to me.

AThousandShips · 2023-06-11T12:07:41Z

There's still reviews that need to be done before this can be merged

rohanrhu · 2023-06-11T20:03:34Z

Aren't we able to use breakpoints in threads with this PR?

Calinou · 2023-06-11T20:20:49Z

Aren't we able to use breakpoints in threads with this PR?

Yes, but 4.1 is in feature freeze now. While this PR fixes a bug, it still has a large scope and may be postponed to 4.2.

Faless

I think the overall debugger changes are good (see my comment about locking in _get_message).

One thing I noticed, is that breaking in a thread will spam the console with errors like:

This function in this node (/root/Control) can only be accessed from either the main thread or a thread group. Use call_deferred() instead.

As far as I can tell, this is due to the Remote Inspector trying to get the Node properties if a local reference to a node is present in the thread stack.

This is the test I used:

extends Control

var t = Thread.new()


func _tfunc():
	while true:
		var v1 := 1
		var v2 := "Test"
		print("Test")
		OS.delay_msec(1000)


func _ready():
	t.start(_tfunc)


func _process(delta):
	var v1 := "Test"
	print("Delta")

I think that the best solution here would be to change all the scene: messages (the ones handled by SceneDebugger) to always be sent to the main thread.

EDIT: Well, it seems like it's already the case that scene: messages are sent to the main thread, so it might be a different message that generate the object spam... I'll try to dig deeper.

EDIT2: The error seems to be coming from the stack variables serialization. We are passing full objects (and not encoded ID), and the remote inspector uses the received values directly (instead of the 2-steps process which we use when inspecting remote tree). This might require some changes to how we handle those.

core/debugger/remote_debugger.cpp

Faless · 2023-07-22T14:17:53Z

This patch should fix the error spam, while still allow for remote inspection:

diff --git a/core/debugger/debugger_marshalls.cpp b/core/debugger/debugger_marshalls.cpp
index 591b44869f..3e6b7501c7 100644
--- a/core/debugger/debugger_marshalls.cpp
+++ b/core/debugger/debugger_marshalls.cpp
@@ -67,6 +67,7 @@ Array DebuggerMarshalls::ScriptStackVariable::serialize(int max_size) {
 	Array arr;
 	arr.push_back(name);
 	arr.push_back(type);
+	arr.push_back(value.get_type());
 
 	Variant var = value;
 	if (value.get_type() == Variant::OBJECT && value.get_validated_object() == nullptr) {
@@ -74,7 +75,7 @@ Array DebuggerMarshalls::ScriptStackVariable::serialize(int max_size) {
 	}
 
 	int len = 0;
-	Error err = encode_variant(var, nullptr, len, true);
+	Error err = encode_variant(var, nullptr, len, false);
 	if (err != OK) {
 		ERR_PRINT("Failed to encode variant.");
 	}
@@ -88,11 +89,12 @@ Array DebuggerMarshalls::ScriptStackVariable::serialize(int max_size) {
 }
 
 bool DebuggerMarshalls::ScriptStackVariable::deserialize(const Array &p_arr) {
-	CHECK_SIZE(p_arr, 3, "ScriptStackVariable");
+	CHECK_SIZE(p_arr, 4, "ScriptStackVariable");
 	name = p_arr[0];
 	type = p_arr[1];
-	value = p_arr[2];
-	CHECK_END(p_arr, 3, "ScriptStackVariable");
+	var_type = p_arr[2];
+	value = p_arr[3];
+	CHECK_END(p_arr, 4, "ScriptStackVariable");
 	return true;
 }
 
diff --git a/core/debugger/debugger_marshalls.h b/core/debugger/debugger_marshalls.h
index 8ba93c3092..1b81623688 100644
--- a/core/debugger/debugger_marshalls.h
+++ b/core/debugger/debugger_marshalls.h
@@ -38,6 +38,7 @@ struct DebuggerMarshalls {
 		String name;
 		Variant value;
 		int type = -1;
+		int var_type = -1;
 
 		Array serialize(int max_size = 1 << 20); // 1 MiB default.
 		bool deserialize(const Array &p_arr);
diff --git a/editor/debugger/editor_debugger_inspector.cpp b/editor/debugger/editor_debugger_inspector.cpp
index e083e1746d..1e9b7c2c60 100644
--- a/editor/debugger/editor_debugger_inspector.cpp
+++ b/editor/debugger/editor_debugger_inspector.cpp
@@ -232,7 +232,7 @@ void EditorDebuggerInspector::add_stack_variable(const Array &p_array) {
 	PropertyHint h = PROPERTY_HINT_NONE;
 	String hs;
 
-	if (v.get_type() == Variant::OBJECT) {
+	if (var.var_type == Variant::OBJECT) {
 		v = Object::cast_to<EncodedObjectAsID>(v)->get_object_id();
 		h = PROPERTY_HINT_OBJECT_ID;
 		hs = "Object";

core/debugger/engine_debugger.h

* This implementation adds threads on the side of the client (script debugger). * Some functions of the debugger are optimized. * The profile is also now thread safe using atomics. * The editor can switch between multiple threads when debugging. This PR adds threaded support for the script language debugger. Every thread has its own thread local data and it will connect to the debugger using multiple thread IDs. This means that, now, the editor can receive multiple threads entering debug mode at the same time.

reduz · 2023-07-26T10:12:59Z

@Faless @AThousandShips Thanks lots!

Done

RandomShaper

I've checked this, not as deeply as I would have wanted due to my current limited bandwitdth. I spotted no issues. In case some bugs arise, I may be able to be of greater help in fixing them.

vnen

Looks good to me. I tested for a bit and seems to work well.

YuriSizov · 2023-07-26T16:50:36Z

Thanks!

reduz marked this pull request as ready for review April 29, 2023 15:24

reduz requested review from a team as code owners April 29, 2023 15:24

adamscott reviewed Apr 29, 2023

View reviewed changes

core/debugger/remote_debugger.cpp Show resolved Hide resolved

adamscott reviewed Apr 29, 2023

View reviewed changes

editor/debugger/script_editor_debugger.cpp Outdated Show resolved Hide resolved

adamscott reviewed Apr 29, 2023

View reviewed changes

core/debugger/remote_debugger.cpp Outdated Show resolved Hide resolved

Calinou added enhancement topic:gdscript topic:editor labels Apr 29, 2023

Calinou added this to the 4.x milestone Apr 29, 2023

Calinou removed the topic:editor label Apr 29, 2023

reduz force-pushed the threaded-debugger branch from a88b1c2 to d765290 Compare April 30, 2023 12:11

reduz force-pushed the threaded-debugger branch 2 times, most recently from f907f7d to 114ebea Compare April 30, 2023 14:36

Calinou previously requested changes May 1, 2023

View reviewed changes

vnen reviewed May 2, 2023

View reviewed changes

modules/gdscript/gdscript.cpp Show resolved Hide resolved

vnen reviewed May 2, 2023

View reviewed changes

editor/debugger/script_editor_debugger.cpp Outdated Show resolved Hide resolved

reduz force-pushed the threaded-debugger branch from 114ebea to 6b17667 Compare May 4, 2023 10:49

reduz requested a review from a team as a code owner May 4, 2023 10:49

Calinou mentioned this pull request May 4, 2023

Remove outdated notices about GDScript debugger not supporting threads godotengine/godot-docs#7278

Merged

adamscott approved these changes May 8, 2023

View reviewed changes

akien-mga requested review from Calinou and vnen May 9, 2023 06:12

akien-mga added the bug label May 9, 2023

akien-mga added this to the 4.1 milestone May 9, 2023

This comment was marked as off-topic.

Sign in to view

akien-mga requested review from Faless and RandomShaper June 12, 2023 09:33

akien-mga modified the milestones: 4.1, 4.2 Jun 19, 2023

This was referenced Jun 19, 2023

Show errors from non-main threads in editor #63267

Closed

Add thread support to debugger #64364

Closed

adamscott requested a review from dalexeev July 8, 2023 14:40

dalexeev removed their request for review July 8, 2023 18:12

akien-mga mentioned this pull request Jul 11, 2023

Threads do not output errors to debugger #18330

Closed

Faless reviewed Jul 22, 2023

View reviewed changes

core/debugger/remote_debugger.cpp Show resolved Hide resolved

AThousandShips reviewed Jul 22, 2023

View reviewed changes

core/debugger/engine_debugger.h Outdated Show resolved Hide resolved

reduz force-pushed the threaded-debugger branch from 6b17667 to 5e512b7 Compare July 26, 2023 10:07

RandomShaper reviewed Jul 26, 2023

View reviewed changes

vnen approved these changes Jul 26, 2023

View reviewed changes

YuriSizov merged commit c4e5822 into godotengine:master Jul 26, 2023

akien-mga mentioned this pull request Aug 9, 2023

Avoid retrieving the object ID of a stack variable if it is nil #80256

Merged

AThousandShips mentioned this pull request Nov 30, 2023

assert from a thread does not function as expected #85523

Closed

AThousandShips mentioned this pull request Jan 6, 2024

Implement a threaded script debugger godotengine/godot-proposals#4952

Closed

Zylann mentioned this pull request Apr 27, 2024

GDScript breakpoints cause VoxelGeneratorScript to stop working Zylann/godot_voxel#628

Closed

pafuent mentioned this pull request Aug 25, 2024

Audio does not stop when pausing the project from the editor #68880

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support threads in the script debugger #76582

Support threads in the script debugger #76582

reduz commented Apr 29, 2023 •

edited by akien-mga

Loading

adamscott commented Apr 30, 2023

reduz commented Apr 30, 2023 •

edited

Loading

Chaosus commented May 1, 2023

Calinou left a comment •

edited

Loading

vnen commented May 2, 2023

vnen commented May 2, 2023

reduz commented May 4, 2023

adamscott left a comment

This comment was marked as off-topic.

AThousandShips commented Jun 11, 2023

rohanrhu commented Jun 11, 2023 •

edited

Loading

Calinou commented Jun 11, 2023

Faless left a comment •

edited

Loading

Faless commented Jul 22, 2023

reduz commented Jul 26, 2023

RandomShaper left a comment

vnen left a comment

YuriSizov commented Jul 26, 2023

Support threads in the script debugger #76582

Support threads in the script debugger #76582

Conversation

reduz commented Apr 29, 2023 • edited by akien-mga Loading

adamscott commented Apr 30, 2023

reduz commented Apr 30, 2023 • edited Loading

Chaosus commented May 1, 2023

Calinou left a comment • edited Loading

Choose a reason for hiding this comment

vnen commented May 2, 2023

vnen commented May 2, 2023

reduz commented May 4, 2023

adamscott left a comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

AThousandShips commented Jun 11, 2023

rohanrhu commented Jun 11, 2023 • edited Loading

Calinou commented Jun 11, 2023

Faless left a comment • edited Loading

Choose a reason for hiding this comment

Faless commented Jul 22, 2023

reduz commented Jul 26, 2023

RandomShaper left a comment

Choose a reason for hiding this comment

vnen left a comment

Choose a reason for hiding this comment

YuriSizov commented Jul 26, 2023

reduz commented Apr 29, 2023 •

edited by akien-mga

Loading

reduz commented Apr 30, 2023 •

edited

Loading

Calinou left a comment •

edited

Loading

rohanrhu commented Jun 11, 2023 •

edited

Loading

Faless left a comment •

edited

Loading