Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show errors from non-main threads in editor #63267

Closed
wants to merge 1 commit into from

Conversation

derammo
Copy link
Contributor

@derammo derammo commented Jul 21, 2022

The GDScript VM breaks out of the current function on error, such as assertion failures or general script errors. These errors cause debug breakpoints when running under the debugger/editor. However, threads that are not the main thread are not able to be debugged. For any worker threads, these errors were just printed to stderr, and then the thread would silently exit the current frame, leading to unpredictable results. There was no indication in the Editor itself.

This change submits such errors to the error reporting facility of the debugger also, so they can be displayed in the editor as Errors. They still don't cause debug breakpoints, because that would require general support for thread debugging. This is for notifications only.

Example program:

extends Node

var typed: int 

func _ready():
	var thread = Thread.new()
	thread.start(_thread_function)
	thread.wait_to_finish()
	
func _thread_function():
	assert(typed > 0, "this was only shown on stderr before, with no indication in the Editor")

image

@derammo derammo marked this pull request as ready for review July 21, 2022 04:20
@derammo derammo requested review from a team as code owners July 21, 2022 04:20
@akien-mga
Copy link
Member

akien-mga commented Jul 21, 2022

Does this solve #2446 ?

Edit: I commented before reading the PR, it does not:

They still don't cause debug breakpoints, because that would require general support for thread debugging. This is for notifications only.

@akien-mga akien-mga added this to the 4.0 milestone Jul 21, 2022
@derammo
Copy link
Contributor Author

derammo commented Jul 21, 2022

@akien-mga this PR is me learning the code base around VM and debugger enough to go work on 2446...

@derammo
Copy link
Contributor Author

derammo commented Sep 15, 2022

@Faless can I close this? You mentioned elsewhere that you thought it was wrong, but that may have been a misunderstanding.

@akien-mga akien-mga modified the milestones: 4.0, 4.1 Feb 13, 2023
@adamscott
Copy link
Member

CC @Faless @vnen Is it mergeable/safe enough for 4.1? If not, I'll push the milestone to 4.2.

@akien-mga
Copy link
Member

CC @Faless @vnen Is it mergeable/safe enough for 4.1? If not, I'll push the milestone to 4.2.

This may be superseded by #76582 which is planned for 4.2. Once that one is merged, this one should be re-evaluated to see if it's still relevant (partially or in full).

@akien-mga akien-mga modified the milestones: 4.1, 4.2 Jun 19, 2023
@AThousandShips AThousandShips changed the title errors from additional threads now shown in editor Show errors from non-main threads in editor Nov 9, 2023
@AThousandShips
Copy link
Member

Seeing how #76582 is merged, is this still relevant?

@akien-mga
Copy link
Member

The reproduction case from the OP is indeed well supported in 4.2-beta:

image

So this doesn't seem needed anymore. Thanks for the contribution!

@akien-mga akien-mga closed this Nov 9, 2023
@AThousandShips AThousandShips removed this from the 4.2 milestone Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants