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 editor error reporting using resource loader thread's call queues #92426

Merged
merged 1 commit into from
Jun 3, 2024

Conversation

RandomShaper
Copy link
Member

@RandomShaper RandomShaper commented May 27, 2024

After #91630, deferred calls happening during a resource load going on a non-main thread, are flushed on that very thread. As a result, the mesaures in place for thread-safety so that the editor UI is not updated from a non-main thread need an update, which this PR performs.

Formerly, it was enough for those pieces of code to defer the call in case the caller thread wasn't the main one. However, given the potential flush-on-non-main-thread situation, that second-hand call must explicitly be pushed to the main call queue so it eventually happend on the main thread.

In a way, this requirement introduced by #91630 is a compat breakage. However, I'd say the surface is very small and it seems to be fully solvable in the editor code itself. Maybe some extensions or plugins need to be aware of this as well and perform the appropriate changes, though.

Fixes EditorToaster deadlock in #85255 (comment).

NOTE: I haven't tested this myself.

@RandomShaper
Copy link
Member Author

@AThousandShips, thanks, but in the end I've decided to leave the .bind()s there, so the scope of this PR is only about where to send the call to.

@AThousandShips
Copy link
Member

Fair enough, but they shouldn't have used bind in the first place but just call_deferred(...)

@RandomShaper
Copy link
Member Author

I'm not sure many are aware of that being possible. I wasn't until very recently myself.

@Hilderin
Copy link
Contributor

I tested the code of this PR, and now the project is open without problem. I see the errors toasts on startup and the editor does not hang.
Thanks!

@akien-mga
Copy link
Member

akien-mga commented Jun 3, 2024

I tested this PR on Linux, it seems to fix the linked issue partially.

It fixes the potential deadlock reported by @Hilderin in #85255 (comment) with the linked MRP.

It doesn't seem to fix the deadlock reported by @danny88881 with the MRP in the OP: #85255 (comment)

With this PR, I still have this kind of lock on closing the MRP:

(gdb) bt
#0  0x00007ffff7dae5e3 in clock_nanosleep@GLIBC_2.2.5 () from /lib64/libc.so.6
#1  0x00007ffff7dba387 in nanosleep () from /lib64/libc.so.6
#2  0x0000000007397507 in OS_Unix::delay_usec (this=0x7fffffffd0e0, p_usec=1000) at ./drivers/unix/os_unix.cpp:289
#3  0x000000000a6b4c8b in ResourceLoader::clear_thread_load_tasks () at ./core/io/resource_loader.cpp:1082
#4  0x0000000005b9b4eb in Main::cleanup (p_force=false) at main/main.cpp:4216
#5  0x0000000005adcf51 in main (argc=1, argv=0x7fffffffd738) at platform/linuxbsd/godot_linuxbsd.cpp:89

Overall it seems to me the two MRPs in #85255 are not showing the same bug. The one in the OP isn't related to the EditorToaster, it happens at runtime outside the editor.

@akien-mga akien-mga merged commit ef886b0 into godotengine:master Jun 3, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@RandomShaper RandomShaper deleted the fix_ed_toast_mt branch June 4, 2024 08:14
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.

None yet

4 participants