Provide a way to attach a c++ thread to the JVM

[sonicdebris]

In our application we are calling java listeners from a background thread, and we faced issues similar to #238, #219, #176 and #141 . As far as I understand, the main concern for not using AttachCurrentThread in Djinni is because of uncertainty about when to call DetachCurrentThread.

Actually, as adviced on the NDK website's Jni Tips page, it would be possible to reliably detach the thread when it exits.

I've already used this mechanism in a few big android projects and it's working quite well. One needs to define a thread exit callback, create a pthread_key once per process (a good place could be where the jvm is cached)

static pthread_key_t threadKey;

void onThreadExit(void*)
{
    jvm->DetachCurrentThread();
}

void createThreadKey()
{
    pthread_key_create(&threadKey, onThreadExit);
}

and then lazily attach the thread to the jvm and associate the key to the thread itself, the first time the env is needed for that thread, something like:

JNIEnv* getEnv()
{
    JNIEnv* env;
    jint res = jvm->GetEnv(reinterpret_cast<void**>(&env), JNI_VERSION_1_6);

    if (res == JNI_EDETACHED)
    {
        res = jvm->AttachCurrentThread(&env, nullptr);
        assert(res == JNI_OK);
        pthread_setspecific(threadKey, env);
    }

    return env;
}

For now we are calling this manually when creating a thread where we know java methods will be called using djinni, and it's working fine, so maybe we could try integrating this logic directly in djinni's jniGetThreadEnv (so we can avoid caching the jvm ourselves, too, and having to stub the function for non-android platforms).

If you are interested, I could submit a pull request with the changes.

[artwyman]

That sounds like a reasonable approach. It does seem to make some assumptions about the platform, though, and I'm not educated enough on pthreads to know exactly what the safe vs. unsafe cases would be. E.g. I'm not sure whether threads created in ways other than pthreads are guaranteed to call pthread_exit to do this kind of cleanup. Also use of pthreads at all would seem to rule out Windows (or anything non-POSIX) as a platform. Android is indeed the primary target platform for Djinni's Java support, but it's meant to generalize to any Java platform (the unit tests run on OSX or Linux for instance).

I wouldn't necessarily rule out doing this, and I'd certainly encourage you to go ahead and edit your local copy of the support-library as appropriate for your own use. But these kind of assumptions about the platform and its usage are why we originally kept threads out of the purview of Djinni. When we designed Djinni, we at Dropbox had seen enough problems with use of native-created threads interacting with Java that we decided to let Java create all of the threads, and created the PlatformThreads abstraction to do so, which you can see here: https://github.com/dropbox/djinni/tree/master/extension-libs/platform-threads

It's possible the situation has gotten better, or there are other workrounds to the issues we saw (I recall issues with class loaders), but I haven't gone back to re-test other approaches.

[sonicdebris]

Ah, I am too android-centric and always forget about windows and non-posix environments!

I came up with a more portable solution using std::thread's local storage. It's even simpler, you would just need to call the following function when attaching the thread:

void detachThreadOnExit()
{
    thread_local struct OnExit {
        ~OnExit() {
            g_cachedJVM->DetachCurrentThread();
        }
    } onExit;
}

Regarding the class loader, according to the same Jni Tips page (and to my own experience), it will be fine if java classes are found and cached directly in JNI_onLoad, as I understand to be the case with Djinni.

[artwyman]

Last time I checked (which admittedly was several years ago) thread_local wasn't yet supported by the compilers we were using on mobile. I think it was Android which wasn't up-to-date yet, and I know Dropbox hasn't updated our NDK for a while so that might still be an issue. I suspect that with a migration to clang+libc++ this approach could work with the NDK.