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

Let user know about dead instances in deferred calls (reverted) #78987

Merged
merged 1 commit into from
Jul 26, 2023

Conversation

RandomShaper
Copy link
Member

So users have a hint about why their games print weird error messages or crash on release.

Fixes #75802.
Maybe fixes #78788.
Maybe fixes #70910.

@RandomShaper RandomShaper added this to the 4.2 milestone Jul 3, 2023
@RandomShaper RandomShaper requested a review from a team as a code owner July 3, 2023 15:44
@RandomShaper RandomShaper force-pushed the err_bad_deferred_target branch from e6b9af8 to 3a6527d Compare July 3, 2023 15:57
@RandomShaper RandomShaper marked this pull request as draft July 3, 2023 15:59
@RandomShaper RandomShaper marked this pull request as ready for review July 3, 2023 16:09
@RandomShaper RandomShaper added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jul 4, 2023
@RandomShaper
Copy link
Member Author

This one would be good for 4.2 dev builds.

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

This makes sense and seems very beneficial

@YuriSizov YuriSizov merged commit 2c8cbcd into godotengine:master Jul 26, 2023
@YuriSizov
Copy link
Contributor

Thanks!

@RandomShaper RandomShaper deleted the err_bad_deferred_target branch July 26, 2023 17:31
@YuriSizov
Copy link
Contributor

So we also get a lot of "ERROR: Trying to execute a deferred call/notification/set on a previously freed instance. Consider using queue_free() instead of free()." from running tests (you can see the CI run for this very PR). How should we avoid that?

@YuriSizov
Copy link
Contributor

We also get this error in an empty Project Manager on startup. And I get it from simply opening a project in the editor too.

WindowsTerminal_2023-07-28_14-20-18

@reduz
Copy link
Member

reduz commented Jul 29, 2023

I am sorry, but this also has to be either reverted or changed. It is far too common for users to run into a situation where this will throw an error. This will frustrate more users than it will help because the errors are unfixable most of the time.

As example, user call_deferred() during process because they want to wait until something else happens. Also during process of another node it decides its time to complete the level and wipe the scene, or wipe the node that did call_deferred.
It's totally fine this call does not happen because there is no more reason for it to, and it should not error.

Or another situation, you want to merge a lot potential calls and only execute the code ones (like, Control does this when you do queue_redraw() ). In this case, if the node is removed, its also completely fine the redraw does not happen and no error is shown. Most of the code in the engine using call_deferred() expects this behavior because it is what makes the most sense in those contexts. If it throws an error, it is entirely beyond the user ability to fix this error.

I know the suggestion is to use queue_free but this will not do because we can't assume a warranty that messages will be processed after or before the node is freed. As an example, the call_deferred function may happen in uninitialize code or on child removed code while the object is already being deleted and you still want to execute it (GUI resize and redraw notifications as example), and the deletion will still happen before the call queue is processed. Or, sometimes, many resources use call_deferred for updating (as example the primitive meshes) and they will be freed when refcount goes to zero, not with queue_free. Hence they will trigger this error too in an unavoidable way.

I did not add error checking for this specific code for this reason here, which is that its extremely difficult to prevent these situations in a lot of scenarios. They just happen due to natural circumstances and the user should not receive an error. Error checking for the call itself (function name, arguments, etc) is fine and should remain because if this is wrong, then this is most likely unintended.

Additionally, the engine uses these functions internally a lot so users will run into cases where they can't simply mute the error anyway.

@reduz
Copy link
Member

reduz commented Jul 29, 2023

I propose the following courses of action:

If you really want this, it should be called call_deferred_valid_instance() or something like that and make this check optional.

@reduz
Copy link
Member

reduz commented Jul 29, 2023

Remember that also we have method.call_deferred() in callable too, so whathever is done, a new version of this needs to be added there too. Also on the scene multithreading code. Adding validated/unvalidated versions sounds like a lot of work.

@YuriSizov
Copy link
Contributor

This is reverted by #80081.

@AThousandShips
Copy link
Member

NVM realized #80081 still fails with that, I'm too tired to think clearly atm

@YuriSizov
Copy link
Contributor

@AThousandShips What are you referring to?

@AThousandShips
Copy link
Member

Went down a wrong rabbit hole with wondering if the log was flooded by this causing the space issues for the godot-cpp build, accidentally deleted instead of editing and only this comment got left

@AThousandShips
Copy link
Member

Worth seeing if it reduces the frequency, but realized both that this occured before this got merged and also affects the revert PR so not directly causal

@jordanlis
Copy link

Has it been merged to 4.2dev2 ? I had no issues with 4.2dev1, now my code is buggy with this when I use set_deffered, I don't understand how to solve it.

Here's a conversation that I found on reddit for information https://www.reddit.com/r/godot/comments/15jhppb/please_help_constant_issues_with_queuefree/?sort=new

@AThousandShips
Copy link
Member

AThousandShips commented Aug 8, 2023

It has been reverted in #80081, so it will be fixed in dev3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment