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

Avoid crash on exiting due to late prints #80161

Merged

Conversation

RandomShaper
Copy link
Member

@RandomShaper RandomShaper commented Aug 2, 2023

Fixes #80152.

UPDATE: Probably fixes #79379.
Bugsquad edit: And fixes #79506.

The MRP there adds a message when the EditorNode has already been destroyed. The most obvious fix is done here, which is just validating the singleton when a deferred call happens. The problem is that static calls in the message queue can't be validated.

Now, there are potential alternatives to this fix that may be done instead of in addition:

a) Let the static callables have some sort of custom validation.
For instance, there's this call:
callable_mp_static(&EditorNode::_print_handler_impl).bind(p_string, p_error, p_rich).call_deferred();
With this approach it could become something like this:
callable_mp_static(&EditorNode::_print_handler_impl).bind(p_string, p_error, p_rich).add_validator([]() -> bool{ return EditorNode::get_singleton(); }).call_deferred();

b) Like a), but using a sentinel pointer:
callable_mp_static(&EditorNode::_print_handler_impl).bind(p_string, p_error, p_rich).add_validity_sentinel(&EditorNode::singleton).call_deferred();

c) Make all deferred calls of this kind via the object instance instead of static. That would enable the current validated call mechanism to do its magic.

@AThousandShips
Copy link
Member

Does this fix #79379?

@RandomShaper
Copy link
Member Author

Does this fix #79379?

Very likely.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes a lot of sense to cleanup the singleton like this.

@akien-mga
Copy link
Member

CC @KoBeWi for the discussion on static callable validation - but this PR is ready to merge as is IMO.

@KoBeWi
Copy link
Member

KoBeWi commented Aug 2, 2023

If a static function depends on an instance, it shouldn't be static, no?
Also if it's expected that the instance it involves can be invalidated, the check should be inside the function itself I think.

editor/editor_node.cpp Outdated Show resolved Hide resolved
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still seems good after changes.

@YuriSizov YuriSizov merged commit 79f6ac5 into godotengine:master Aug 2, 2023
14 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants