NativeScript / canvas

Apache License 2.0
88 stars 18 forks source link

Android: Infinite loop keeps old GLThread alive #70

Closed CatchABus closed 2 years ago

CatchABus commented 2 years ago

I noticed there is this piece of code when running GLThread in android. https://github.com/NativeScript/canvas/blob/master/packages/canvas/src-native/canvas-android/canvas/src/main/java/org/nativescript/canvas/GLContext.kt#L599

        while (true) {
        try {
            if (!isPaused) {
                makeEGLContextCurrent()
                mQueue.take().run()
            }
        } catch (e: InterruptedException) {
            break
        }
    }
    deInitEGL()

When leaving the page that contains the canvas, it seems that property isPaused is set to true. However, infinite loop will continue to execute, keeping thread alive. This also makes sure that deInitEGL() is never called.

I tested this by adding a log message inside loop and navigating back and forth between app pages to confirm that a new GLThread was created while old thread was still alive, printing my message.

Perhaps, we should terminate thread when native view is disposed or during NS view unload? I'm not sure which is the best. I believe GLThread should have a property called isRunning and thread loop should make use of it. For example:

while (isRunning)
{
    ...
}

This property could be set to false in order to finish loop and stop thread. It should also be a better approach than thread interrupt() since we want to gracefully terminate thread.

triniwiz commented 2 years ago

Interesting I'll try updating this, also which version of core was this tried on ?

CatchABus commented 2 years ago

Interesting I'll try updating this, also which version of core was this tried on ?

I used the demo of latest version of this repo.

CatchABus commented 2 years ago

@triniwiz There is also the case of a HandlerThread, in case Canvas uses CPU: https://github.com/NativeScript/canvas/blob/master/packages/canvas/src-native/canvas-android/canvas/src/main/java/org/nativescript/canvas/TNSCanvas.kt#L188

Perhaps, this thread needs a quitSafely method call when canvas is disposed.

triniwiz commented 2 years ago

Should be good with https://github.com/NativeScript/canvas/commit/4d87d63452d27690e9ddf0f0baa5cbad84cd8644