Control jvm thread on our side #26
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
britzl
reviewed
Aug 31, 2022
|
@britzl sure! I added explanation in the first post |
AGulev
commented
Aug 31, 2022
| @@ -27,9 +29,11 @@ void FirebaseAnalytics_Safe_LogEvent(lua_State*, const char* name, const firebas | |||
| { | |||
| LogEvent(name, params, number_of_parameters); | |||
| } | |||
| #define THREAD_ATTACHER() dmAndroid::ThreadAttacher threadAttacher; | |||
| #else | |||
| #define THREAD_ATTACHER() | |||
There was a problem hiding this comment.
This define helps to avoid multiple #if defined(DM_PLATFORM_ANDROID) checks and makes code clear
britzl
approved these changes
Sep 1, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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_IsAttachedflag)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:
I wasn't sure if
no-opin this case meansdoes nothingorcosts nothing. I checked Android source code where I found that environment pointer still returned and it isJNI_OKoperation. Sono-opmeans costs nothing (does nothing as well, but returning of the pointer is kinda operation).