defold / extension-firebase-analytics

Google Firebase Analytics extension for the Defold game engine
https://www.defold.com/extension-firebase-analytics/
MIT License
19 stars 12 forks source link

Control jvm thread on our side #26

Closed AGulev closed 2 years ago

AGulev commented 2 years ago

Now we have the following problem: Firebase attach thread and detach only when the main thread shut down (if shutdown thread without detach it will crash the app). Firebase does it safe - it attaches thread before detaching to avoid situation of attempt to detach thread which isn't attached.

It looks ok: just attach thread when you need it and don't detach - because you don't know who needs it except you.

But we use a bit different pattern for that. Our ThreadAttacher checks if thread attached or not, if it's not attached - then this particular attacher is "owner" and have to detach thread (see code m_IsAttached flag)

That means that even if we have create 10 ThreadAttacher instances only one instance will detach thread - the instance who attached the thread "the owner". But in case when the thread was attached by another code our ThreadAttacher will never be the owner and will never detach the thread. That's why we have this leak.

My fix just keep management of the thread on our side using ThreadAttacher. If our ThreadAttacher attached thread - then it will detach. But what about multiple call of the AttachCurrentThread()?

According to the documentation:

Attaching a natively-created thread causes a java.lang.Thread object to be constructed and added to the “main” ThreadGroup, making it visible to the debugger. Calling AttachCurrentThread on an already-attached thread is a no-op.

I wasn't sure if no-op in this case means does nothing or costs nothing. I checked Android source code where I found that environment pointer still returned and it is JNI_OK operation. So no-op means costs nothing (does nothing as well, but returning of the pointer is kinda operation).

AGulev commented 2 years ago

@britzl sure! I added explanation in the first post