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

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

Open
bghgary opened this issue Apr 19, 2023 · 6 comments
Open
Assignees
Labels
bug Something isn't working
Milestone

Comments

@bghgary
Copy link
Contributor

bghgary commented Apr 19, 2023

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.

@bghgary bghgary added the bug Something isn't working label Apr 20, 2023
@thomlucc thomlucc added this to the 7.0 milestone Apr 25, 2023
@bghgary bghgary self-assigned this Sep 8, 2023
@CedricGuillemet
Copy link
Contributor

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

@bghgary
Copy link
Contributor Author

bghgary commented Nov 27, 2023

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
Copy link

zxffffffff commented Jan 12, 2024

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
Copy link
Contributor

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

@zxffffffff
Copy link

zxffffffff commented Jan 13, 2024

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.

@thomlucc thomlucc modified the milestones: 7.0, 8.0 Mar 26, 2024
@CedricGuillemet
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants