godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
91.13k stars 21.19k forks source link

[4.1.dev.3] Multiplayer game crash: multiplayer can only be manipulated from the main thread #77707

Closed aldocd4 closed 1 year ago

aldocd4 commented 1 year ago

Godot version

v4.1.dev3.mono.official [a67d37f7c]

System information

Windows 11 - v4.1.dev3 - Vulkan

Issue description

Hello,

I'm playing with the latest 4.1 dev snapshot (3) and C#, and it seems that my multiplayer game crashes with the following stacktrace:

ERROR: Multiplayer can only be manipulated from the main thread.
   at: (scene/main/scene_tree.cpp:1477)
Fatal error. System.AccessViolationException: Attempted to read or write protected memory. This is often an indication that other me.
   at Godot.NativeInterop.NativeFuncs.godotsharp_method_bind_call(IntPtr, IntPtr, Godot.NativeInterop.godot_variant**, Int32, Godot.)
   at Godot.NativeCalls.godot_icall_3_676(IntPtr, IntPtr, Int64, Godot.NativeInterop.godot_string_name, Godot.Variant[])
   at Godot.Node.RpcId(Int64, Godot.StringName, Godot.Variant[])
   at FightHandler.ServerUpdateFightStep(Evael.Scripts.Game.Fight.Fight)
   at Evael.Scripts.Game.Fight.Fight.OnTimerTick(System.Object, System.Timers.ElapsedEventArgs)
   at System.Timers.Timer.MyTimerCallback(System.Object)
   at System.Threading.ExecutionContext.RunInternal(System.Threading.ExecutionContext, System.Threading.ContextCallback, System.Obje)
   at System.Threading.TimerQueueTimer.Fire(Boolean)
   at System.Threading.TimerQueue.FireNextTimers()
   at System.Threading.ThreadPoolWorkQueue.Dispatch()
   at System.Threading.PortableThreadPool+WorkerThread.WorkerThreadStart()

It's working correctly on 4.1 dev 1 and 4.1 dev 2 snapshots.

Steps to reproduce

A timer that trigger a simple function making an rpc call:

var timer = new Timer(14000);
timer.Elapsed += (object sender, ElapsedEventArgs e) =>
{
    RpcId(peerId, "UpdateFightStep", 1);
};
timer.AutoReset = true;
timer.Start();

Minimal reproduction project

I will try to reproduce it in a small project.

aldocd4 commented 1 year ago

Important note: the timer used here is the C# timer (System.Timers) and not the godot timer node.

I believe this bug occurs because the native C# timer is not single-threaded. It worked on latest v4.0 stable version and the 2 dev snapshots of 4.1.

Maybe this change is intended and is not a bug.

AThousandShips commented 1 year ago

You need to use other methods as you are not allowed to manipulate it from the main thread, use something like deferred calls, this is because processing got reworked on 4.1.dev I believe

Don't think this is a bug

Edit: My bad, didn't realize this was get_multiplayer, I'm unsure why it has a thread check like that, that might be a bug yes

raulsntos commented 1 year ago

The get_multiplayer method checks if we are in the main thread:

https://github.com/godotengine/godot/blob/d87bdef2a4f7c83c6a236f8916ebac35ae4fd176/scene/main/scene_tree.cpp#L1479

This seems intentional, the PR says:

Multiplayer API is not thread safe.

The multiplayer code (rpc as example) is not thread safe, the Node API does not care about this, so we need to discuss if we want to implement safety at Node level or Multiplayer level.

Using a Godot timer should avoid the issue since it will tick in the main thread. But if you want to keep using C# timers, you should be able to use CallDeferred. You can use GodotObject.CallDeferred or a Callable, here's an example using a Callable:

var timer = new Timer(14000);
timer.Elapsed += (object sender, ElapsedEventArgs e) =>
{
    Callable.From(() => RpcId(peerId, "UpdateFightStep", 1)).CallDeferred();
};
timer.AutoReset = true;
timer.Start();
YuriSizov commented 1 year ago

Closing per comment above (and OP's acknowledgement of it).