Crash when running project with error ERROR: Condition "flushing_cmd_queue != nullptr" is true.

[chrisl8]

Tested versions

• Reproducible in: v4.3.dev.custom_build [e5b4ef8]

System information

Godot v4.3.dev (e5b4ef8) - Windows 10.0.22621 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 3080 Ti (NVIDIA; 31.0.15.5186) - AMD Ryzen 9 5900X 12-Core Processor (24 Threads)

Issue description

Upon running the TestCustomTextures Demo project it crashes with the error:

ERROR: Condition "flushing_cmd_queue != nullptr" is true.
at: WorkerThreadPool::thread_enter_command_queue_mt_flush (core\object\worker_thread_pool.cpp:591)

The TestCustomTextures Demo works in 4.2.1 Stable version.

Steps to reproduce

Open the TestCustomTextures Demo project and attempt to run it.
Note that it opens fine in the editor initially. It only crashes when you attempt to Play it.

Minimal reproduction project (MRP)

Use the TestCustomTextures Demo project.

Bisect results

83241ab is the first bad commit
commit 83241ab

Date: Mon Jan 29 14:47:50 2024 -0800

Enforce calling RenderingDevice code from rendering thread in TextureRD classes

scene/resources/texture_rd.cpp | 186 ++++++++++++++++++++++-------------------
scene/resources/texture_rd.h | 9 ++
2 files changed, 108 insertions(+), 87 deletions(-)

[akien-mga]

83241ab is the first bad commit

That's #87721, CC @clayjohn @RandomShaper.

[clayjohn]

When I run the demo project with dev 5 I just get this warning:

W 0:00:02:0207   setup2: The Multi-Threaded rendering thread model is experimental, and has known issues which can lead to project crashes. Use the Single-Safe option in the project settings instead.
  <C++ Source>   main/main.cpp:2712 @ setup2()

Turning off multi threaded rendering solves the issue.

I can confirm this happens on master and is not fixed by #90268. Will need to debug further

[clayjohn]

Okay, it is crashing on this line

godot/core/templates/command_queue_mt.h

Line 373 in 9d6bdbc

SyncSemaphore *sync_sem = cmd->get_sync_semaphore();

With this signal:

signal SIGSEGV: address not mapped to object (fault address: 0x0)

Call stack:

0 (Unknown Source:0)
CommandQueueMT::_flush() (/home/clayjohn/dev/godot/core/templates/command_queue_mt.h:376)
CommandQueueMT::flush_if_pending() (/home/clayjohn/dev/godot/core/templates/command_queue_mt.h:419)
RenderingServerDefault::texture_replace(RID, RID) (/home/clayjohn/dev/godot/servers/rendering/rendering_server_default.h:201)
Texture2DRD::_set_texture_rd_rid(RID) (/home/clayjohn/dev/godot/scene/resources/texture_rd.cpp:103)
void call_with_variant_args_helper<Texture2DRD, RID, 0ul>(Texture2DRD*, void (Texture2DRD::*)(RID), Variant const**, Callable::CallError&, IndexSequence<0ul>) (/home/clayjohn/dev/godot/core/variant/binder_common.h:304)
void call_with_variant_args<Texture2DRD, RID>(Texture2DRD*, void (Texture2DRD::*)(RID), Variant const**, int, Callable::CallError&) (/home/clayjohn/dev/godot/core/variant/binder_common.h:418)
CallableCustomMethodPointer<Texture2DRD, RID>::call(Variant const**, int, Variant&, Callable::CallError&) const (/home/clayjohn/dev/godot/core/object/callable_method_pointer.h:103)
Callable::callp(Variant const**, int, Variant&, Callable::CallError&) const (/home/clayjohn/dev/godot/core/variant/callable.cpp:57)
CallableCustomBind::call(Variant const**, int, Variant&, Callable::CallError&) const (/home/clayjohn/dev/godot/core/variant/callable_bind.cpp:152)
Callable::callp(Variant const**, int, Variant&, Callable::CallError&) const (/home/clayjohn/dev/godot/core/variant/callable.cpp:57)
Variant Callable::call<>() const (/home/clayjohn/dev/godot/core/variant/variant.h:854)
RenderingServerDefault::_call_on_render_thread(Callable const&) (/home/clayjohn/dev/godot/servers/rendering/rendering_server_default.cpp:388)
CommandQueueMT::Command1<RenderingServerDefault, void (RenderingServerDefault::*)(Callable const&), Callable>::call() (/home/clayjohn/dev/godot/core/templates/command_queue_mt.h:326)
CommandQueueMT::_flush() (/home/clayjohn/dev/godot/core/templates/command_queue_mt.h:377)
CommandQueueMT::flush_all() (/home/clayjohn/dev/godot/core/templates/command_queue_mt.h:423)
RenderingServerDefault::_thread_loop() (/home/clayjohn/dev/godot/servers/rendering/rendering_server_default.cpp:348)
void call_with_variant_args_helper<RenderingServerDefault>(RenderingServerDefault*, void (RenderingServerDefault::*)(), Variant const**, Callable::CallError&, IndexSequence<>) (/home/clayjohn/dev/godot/core/variant/binder_common.h:304)
void call_with_variant_args<RenderingServerDefault>(RenderingServerDefault*, void (RenderingServerDefault::*)(), Variant const**, int, Callable::CallError&) (/home/clayjohn/dev/godot/core/variant/binder_common.h:418)
CallableCustomMethodPointer<RenderingServerDefault>::call(Variant const**, int, Variant&, Callable::CallError&) const (/home/clayjohn/dev/godot/core/object/callable_method_pointer.h:103)

Its crashing after calling texture_replace() which internally requests a flush if called from the server thread. But it is getting called from the server thread because the server thread is flushing

godot/servers/server_wrap_mt_common.h

Lines 246 to 254 in 9d6bdbc

	virtual void m_type(m_arg1 p1, m_arg2 p2) override { \
	WRITE_ACTION \
	if (Thread::get_caller_id() != server_thread) { \
	command_queue.push(server_name, &ServerName::m_type, p1, p2); \
	} else { \
	command_queue.flush_if_pending(); \
	server_name->m_type(p1, p2); \
	} \
	}

[clayjohn]

This diff fixes the crash, but I don't think it is the correct solution:

diff --git a/core/templates/command_queue_mt.h b/core/templates/command_queue_mt.h
index 4056119851..80953ad3cc 100644
--- a/core/templates/command_queue_mt.h
+++ b/core/templates/command_queue_mt.h
@@ -412,7 +412,7 @@ public:
        SPACE_SEP_LIST(DECL_PUSH_AND_SYNC, 15)
 
        _FORCE_INLINE_ void flush_if_pending() {
-               if (unlikely(command_mem.size() > 0)) {
+               if (unlikely(command_mem.size() > 0 && flush_read_ptr == 0)) {
                        _flush();
                }
        }

This skips the flush when another flush is occurring, it works because flush_if_pending is always called from the same thread. However, I think most likely waiting or yielding is the more correct approach here to preserve the order of function calls.