Add nativeSetIntTicketToken JNI implementation in android_build

[TedaLIEz]

nativeSetIntTicketToken in LogManagerProvider.java is incomplete in the last PR: #1131

This PR aims:

1. Implementation of nativeSetIntTicketToken in JNI, which turns out to be the setTicketToken in ILogManager.java
2. Thread-safe in AuthTokensController.cpp to avoid race condition, we found some crashes because of race condition when using this sdk in Android system: Scudo ERROR: race on chunk header

Why adding locks in AuthTokensController.cpp?

We get the token after the app log in one account (MSA, AAD, etc), and our application is a multi-account application. So currently our solution is we cache the token once we get from the server, then every time client calls the logEvent method, it will set the ticket token internally. We don't have the dependencies on logger instanceswhen we in the process of getting token.

We can't guarantee all the logEvent method will be called only in main thread only, so this introduces the multi-threading issues.

[TedaLIEz]

@lalitb Would you like to review this PR? We want to leverage this API to support our telemetry, thanks!

[lalitb]

Looks good. But good to have approval from @anod too.

[lalitb]

Do you expect SetTicketToken to be called after SDK bootstrapping, from multiple thread ? If so, we should also lock in the getter methods.

[TedaLIEz]

Yes, SetTicketToken will be called in multiple threads after we get the logmanager instance. Do you mean we should lock in the GetAuthTokensController method?

getLogManager is already guarded with a lock:

static ILogManager* getLogManager(jlong nativeLogManager)
{
    std::lock_guard<std::mutex> lock(jniManagersMutex);
    if (nativeLogManager < 0 || nativeLogManager >= jniManagers.size())
    {
        return nullptr;
    }

    return jniManagers[nativeLogManager]->manager;
}

[lalitb]

This means we need to lock GetUserTokent(), GetDevicetokens(), GetTickets() methods too. And these methods are called in event upload hot-path. These locks will add the latency in the event upload.

[lalitb]

@maxgolov Do you see any issue in adding the mutex lock in GetUserTokent(), GetDevicetokens(), and GetTickets() methods - which are called in the hot path of event upload?

[maxgolov]

You may need to apply a bit more holistic approach to mutex here:

• mutex works just for the moment while the reference is obtained. But then the collection is returned by reference -> allows for potential concurrent modification and breakage outside of the function call.
• status_t AuthTokensController::Clear() then also needs a mutex.

Ideally the Get* methods should be copying the contents into collection passed by the caller. Then you can use a mutex to guarantee that the copy of this collection is done under the mutex. Otherwise the way as it's currently implemented in the PR - is all unreliable, unfortunately.

[lalitb]

+1

[lalitb]

The CI failures are not related to this PR.

Need to discuss further on mutex lock.

[maxgolov]

The mutex here is kinda pointless: since the reference is returned, outside code could perform concurrent modification of the vector contents without a lock......

[maxgolov]

Maybe this one is good. Please check if it's recursive_mutex, and you'd need to trace the flow where the function is called in the SDK. I honestly don't remember where (it's been 4+ years since I looked into this code).