BabylonJS / BabylonNative

Build cross-platform native applications with the power of the Babylon.js JavaScript framework
MIT License
758 stars 131 forks source link

ScriptLoader will continue loading after AppRuntime/JsRuntime is destroyed #1225

Open bghgary opened 1 year ago

bghgary commented 1 year ago

If ScriptLoader is set up to load a script asynchronously and the app shutdown before the script is loaded, the script loader will continue trying to load the script since there are no cancellation sources associated with the tasks.

Perhaps the ScriptLoader should be owned by JS so that it will know when the JS goes down.

CedricGuillemet commented 8 months ago

@bghgary Is adding a cancellation source enough in this case?

bghgary commented 8 months ago

I don't think so. The problem is that the script loader doesn't know when the JS is going down. The app creates a script loader but doesn't hold on to it. If the app does hold a script loader, then it can work.

zxffffffff commented 6 months ago

This issue is very easy to reproduce: quickly press the 'R' key twice.

Babylon::ScriptLoader loader{runtime};
loader.LoadScript(...);

loader will crash after runtime.reset()

Edit: Can I submit PR to fix this issue? I am studying the source code, I anticipate it will take approximately 1-2 days.

  1. Elevate the lifecycle of ScriptLoader and add loader.reset() before runtime.reset()
  2. Add cancellation to any task of ScriptLoader
CedricGuillemet commented 6 months ago

It's not just scriptloader. I can get a crash with inputs (press R + click in view) image

zxffffffff commented 6 months ago

I cannot reproduce this click issue (occasional bugs?), but there are indeed a series of runtime-related issues in the source code:

  1. ScriptLoader loader{*runtime} obtaining reference to AppRuntime::Dispatch
  2. Plugins obtaining reference to JsRuntime and JsRuntime::Dispatch

I have observed that the interface bindings are currently implemented mainly by saving references. Our goal is to reset the context environment, and in reality, we only need to reset the JS Engine context and clear all tasks accumulated in m_workQueue. It is not necessary to destroy AppRuntime and then recreate it. I suggest AppRuntime created only once, using init & clear member function instead of delete & new.

Edit: Ignoring the issue with the 'R' key, the first problem to address is the synchronization of multiple threads accessing the runtime object. When the runtime is being destroyed, all other threads must synchronize to finish their tasks.

Currently, I have only identified two locations: m_workQueue and ScriptLoader m_task, excluding the main thread.

I suggest adding notification events in AppRuntime, such as onStop(callback), where each plugin actively terminates tasks in the callback function.

CedricGuillemet commented 1 month ago

@bghgary would it be enough to make ScriptLoader::Impl inherit from JsRuntime::IDisposingCallback and make the corresponding plumbing?