From 0254493742d68e47feeaf368a9ad9f8b88b7e6f8 Mon Sep 17 00:00:00 2001 From: heck Date: Mon, 20 Apr 2020 14:06:49 +0200 Subject: [PATCH] Per object c++ mutex locking for the Engine class (java). All calls to an Engine obj are locking the mutex for their respective object. A unordered_map holds the mutex per java obj hash. The mutexes are created/deleted in constructor/destructor of an engine object. A global mutex is being locked for write operations on the unordered map (not thread safe). Cleanup of #includes --- src/basic_api.cc | 61 ++++++++++---- ...oundation_pEp_jniadapter_AbstractEngine.cc | 53 ++++++++---- src/foundation_pEp_jniadapter__Blob.cc | 5 -- src/gen_cpp_Engine.ysl2 | 9 +- src/gen_cpp_Message.ysl2 | 7 -- src/identity_api.cc | 3 - src/jniutils.cc | 55 +++++++++++-- src/jniutils.hh | 39 +++++++-- .../pEp/jniadapter/test/jni88/TestMain.java | 4 +- .../pEp/jniadapter/test/jni92/TestMain.java | 82 +++++++++++-------- 10 files changed, 217 insertions(+), 101 deletions(-) diff --git a/src/basic_api.cc b/src/basic_api.cc index 86be576..f776eab 100644 --- a/src/basic_api.cc +++ b/src/basic_api.cc @@ -1,6 +1,5 @@ #include #include -#include #include #ifndef ANDROID @@ -20,7 +19,9 @@ JNIEXPORT jbyteArray JNICALL Java_foundation_pEp_jniadapter_Engine_trustwords( jobject ident ) { - pEpLog("called"); + pEpLog("called with lock_guard"); + std::lock_guard l(*get_engine_java_object_mutex(env, obj)); + pEp_identity *_ident = to_identity(env, ident); char *words; size_t wsize; @@ -58,7 +59,9 @@ JNIEXPORT jobject JNICALL Java_foundation_pEp_jniadapter_Engine_myself( jobject ident ) { - pEpLog("called"); + pEpLog("called with lock_guard"); + std::lock_guard l(*get_engine_java_object_mutex(env, obj)); + pEp_identity *_ident = to_identity(env, ident); PEP_STATUS status = ::myself(session(), _ident); @@ -77,7 +80,9 @@ JNIEXPORT jobject JNICALL Java_foundation_pEp_jniadapter_Engine_updateIdentity( jobject ident ) { - pEpLog("called"); + pEpLog("called with lock_guard"); + std::lock_guard l(*get_engine_java_object_mutex(env, obj)); + pEp_identity *_ident = to_identity(env, ident); ::update_identity(session(), _ident); @@ -92,7 +97,9 @@ JNIEXPORT jobject JNICALL Java_foundation_pEp_jniadapter_Engine_setOwnKey( jbyteArray fpr ) { - pEpLog("called"); + pEpLog("called with lock_guard"); + std::lock_guard l(*get_engine_java_object_mutex(env, obj)); + pEp_identity *_ident = to_identity(env, ident); char *_fpr = to_string(env, fpr); @@ -113,7 +120,9 @@ JNIEXPORT void JNICALL Java_foundation_pEp_jniadapter_Engine_keyMistrusted( jobject ident ) { - pEpLog("called"); + pEpLog("called with lock_guard"); + std::lock_guard l(*get_engine_java_object_mutex(env, obj)); + pEp_identity *_ident = to_identity(env, ident); if (_ident->fpr == NULL || _ident->fpr[0] == 0) { @@ -137,7 +146,9 @@ JNIEXPORT void JNICALL Java_foundation_pEp_jniadapter_Engine_keyResetTrust( jobject ident ) { - pEpLog("called"); + pEpLog("called with lock_guard"); + std::lock_guard l(*get_engine_java_object_mutex(env, obj)); + pEp_identity *_ident = to_identity(env, ident); if (_ident->fpr == NULL || _ident->fpr[0] == 0) { @@ -161,7 +172,9 @@ JNIEXPORT void JNICALL Java_foundation_pEp_jniadapter_Engine_trustPersonalKey( jobject ident ) { - pEpLog("called"); + pEpLog("called with lock_guard"); + std::lock_guard l(*get_engine_java_object_mutex(env, obj)); + pEp_identity *_ident = to_identity(env, ident); if (_ident->fpr == NULL || _ident->fpr[0] == 0) { @@ -185,7 +198,9 @@ JNIEXPORT void JNICALL Java_foundation_pEp_jniadapter_Engine_trustOwnKey( jobject ident ) { - pEpLog("called"); + pEpLog("called with lock_guard"); + std::lock_guard l(*get_engine_java_object_mutex(env, obj)); + pEp_identity *_ident = to_identity(env, ident); if (_ident->fpr == NULL || _ident->fpr[0] == 0) { @@ -202,7 +217,9 @@ JNIEXPORT void JNICALL Java_foundation_pEp_jniadapter_Engine_importKey( jbyteArray key ) { - pEpLog("called"); + pEpLog("called with lock_guard"); + std::lock_guard l(*get_engine_java_object_mutex(env, obj)); + size_t _size = (size_t) env->GetArrayLength(key); char *_key = (char *) env->GetByteArrayElements(key, NULL); @@ -226,7 +243,9 @@ JNIEXPORT void JNICALL Java_foundation_pEp_jniadapter_Engine_config_1passive_1mo jboolean enable ) { - pEpLog("called"); + pEpLog("called with lock_guard"); + std::lock_guard l(*get_engine_java_object_mutex(env, obj)); + ::config_passive_mode(session(), (bool)enable); } @@ -237,7 +256,9 @@ JNIEXPORT void JNICALL Java_foundation_pEp_jniadapter_Engine_config_1unencrypted jboolean enable ) { - pEpLog("called"); + pEpLog("called with lock_guard"); + std::lock_guard l(*get_engine_java_object_mutex(env, obj)); + ::config_unencrypted_subject(session(), (bool)enable); } @@ -247,7 +268,9 @@ JNIEXPORT void JNICALL Java_foundation_pEp_jniadapter_Engine_blacklist_1add( jbyteArray fpr ) { - pEpLog("called"); + pEpLog("called with lock_guard"); + std::lock_guard l(*get_engine_java_object_mutex(env, obj)); + char *_fpr = to_string(env, fpr); if(_fpr == NULL){ @@ -269,7 +292,9 @@ JNIEXPORT void JNICALL Java_foundation_pEp_jniadapter_Engine_blacklist_1delete( jbyteArray fpr ) { - pEpLog("called"); + pEpLog("called with lock_guard"); + std::lock_guard l(*get_engine_java_object_mutex(env, obj)); + char *_fpr = to_string(env, fpr); if(_fpr == NULL){ @@ -291,7 +316,9 @@ JNIEXPORT jboolean JNICALL Java_foundation_pEp_jniadapter_Engine_blacklist_1is_1 jbyteArray fpr ) { - pEpLog("called"); + pEpLog("called with lock_guard"); + std::lock_guard l(*get_engine_java_object_mutex(env, obj)); + char *_fpr = to_string(env, fpr); bool _listed = 0; @@ -316,7 +343,9 @@ JNIEXPORT jbyteArray JNICALL Java_foundation_pEp_jniadapter_Engine_getCrashdumpL jint maxlines ) { - pEpLog("called"); + pEpLog("called with lock_guard"); + std::lock_guard l(*get_engine_java_object_mutex(env, obj)); + int _maxlines = (int) maxlines; char *_logdata; diff --git a/src/foundation_pEp_jniadapter_AbstractEngine.cc b/src/foundation_pEp_jniadapter_AbstractEngine.cc index 51754d3..0ed33d8 100644 --- a/src/foundation_pEp_jniadapter_AbstractEngine.cc +++ b/src/foundation_pEp_jniadapter_AbstractEngine.cc @@ -1,8 +1,5 @@ #include "foundation_pEp_jniadapter_AbstractEngine.h" -#include #include -#include -#include #include #include #include @@ -28,7 +25,7 @@ jmethodID notifyHandShakeMethodID = nullptr; jmethodID needsFastPollMethodID = nullptr; jmethodID method_values = nullptr; -jobject obj = nullptr; +jobject objj = nullptr; jclass messageClass = nullptr; jclass identityClass = nullptr;; @@ -105,11 +102,11 @@ PEP_STATUS messageToSend(message *msg) pEpLog("############### messageToSend() called"); jobject msg_ = nullptr; - assert(messageClass && messageConstructorMethodID && obj && messageToSendMethodID); + assert(messageClass && messageConstructorMethodID && objj && messageToSendMethodID); msg_ = o.env()->NewObject(messageClass, messageConstructorMethodID, (jlong) msg); - PEP_STATUS status = (PEP_STATUS) o.env()->CallIntMethod(obj, messageToSendMethodID, msg_); + PEP_STATUS status = (PEP_STATUS) o.env()->CallIntMethod(objj, messageToSendMethodID, msg_); if (o.env()->ExceptionCheck()) { o.env()->ExceptionDescribe(); status = PEP_UNKNOWN_ERROR; @@ -158,9 +155,9 @@ PEP_STATUS notifyHandshake(pEp_identity *me, pEp_identity *partner, sync_handsha } } - assert(obj && notifyHandShakeMethodID); + assert(objj && notifyHandShakeMethodID); - PEP_STATUS status = (PEP_STATUS) o.env()->CallIntMethod(obj, notifyHandShakeMethodID, me_, partner_, signal_); + PEP_STATUS status = (PEP_STATUS) o.env()->CallIntMethod(objj, notifyHandShakeMethodID, me_, partner_, signal_); if (o.env()->ExceptionCheck()) { o.env()->ExceptionClear(); return PEP_UNKNOWN_ERROR; @@ -179,14 +176,18 @@ JNIEXPORT void JNICALL Java_foundation_pEp_jniadapter_AbstractEngine_init( ) { pEpLog("called"); + std::lock_guard l(global_mutex); // global mutex for write access to + if (first) { pEpLog("first Engine instance"); first = false; env->GetJavaVM(&jvm); jni_init(); - obj = env->NewGlobalRef(me); + objj = env->NewGlobalRef(me); Adapter::_messageToSend = messageToSend; } + + create_engine_java_object_mutex(env, me); // Create a mutex per java object Adapter::session(); } @@ -196,24 +197,30 @@ JNIEXPORT void JNICALL Java_foundation_pEp_jniadapter_AbstractEngine_release( ) { pEpLog("called"); + std::lock_guard l(global_mutex); // global mutex for write access to + release_engine_java_object_mutex(env, me); Adapter::session(pEp::Adapter::release); } JNIEXPORT jstring JNICALL Java_foundation_pEp_jniadapter_AbstractEngine_getVersion( JNIEnv *env, - jobject + jobject me ) { - pEpLog("called"); + pEpLog("called with lock_guard"); + std::lock_guard l(*get_engine_java_object_mutex(env, me)); + return env->NewStringUTF(::get_engine_version()); } JNIEXPORT jstring JNICALL Java_foundation_pEp_jniadapter_AbstractEngine_getProtocolVersion( JNIEnv *env, - jobject + jobject me ) { - pEpLog("called"); + pEpLog("called with lock_guard"); + std::lock_guard l(*get_engine_java_object_mutex(env, me)); + return env->NewStringUTF(::get_protocol_version()); } @@ -259,7 +266,9 @@ JNIEXPORT void JNICALL Java_foundation_pEp_jniadapter_AbstractEngine_startKeyser jobject me ) { - pEpLog("called"); + pEpLog("called with lock_guard"); + std::lock_guard l(*get_engine_java_object_mutex(env, me)); + pthread_t *thread = nullptr; locked_queue< pEp_identity * > *queue = nullptr; @@ -296,7 +305,9 @@ JNIEXPORT void JNICALL Java_foundation_pEp_jniadapter_AbstractEngine_stopKeyserv jobject me ) { - pEpLog("called"); + pEpLog("called with lock_guard"); + std::lock_guard l(*get_engine_java_object_mutex(env, me)); + pthread_t *thread = nullptr; locked_queue< pEp_identity * > *queue = nullptr; @@ -333,7 +344,9 @@ JNIEXPORT void JNICALL Java_foundation_pEp_jniadapter_AbstractEngine_startSync( jobject me ) { - pEpLog("called"); + pEpLog("called with lock_guard"); + std::lock_guard l(*get_engine_java_object_mutex(env, me)); + pEpLog("######## starting sync"); try { Adapter::startup(messageToSend, notifyHandshake, &o, &JNISync::onSyncStartup, &JNISync::onSyncShutdown); @@ -349,7 +362,9 @@ JNIEXPORT void JNICALL Java_foundation_pEp_jniadapter_AbstractEngine_stopSync( jobject me ) { - pEpLog("called"); + pEpLog("called with lock_guard"); + std::lock_guard l(*get_engine_java_object_mutex(env, me)); + Adapter::shutdown(); } @@ -358,7 +373,9 @@ JNIEXPORT jboolean JNICALL Java_foundation_pEp_jniadapter_AbstractEngine_isSyncR jobject me ) { - pEpLog("called"); + pEpLog("called with lock_guard"); + std::lock_guard l(*get_engine_java_object_mutex(env, me)); + return (jboolean) Adapter::is_sync_running(); } diff --git a/src/foundation_pEp_jniadapter__Blob.cc b/src/foundation_pEp_jniadapter__Blob.cc index 9669c0a..2248708 100644 --- a/src/foundation_pEp_jniadapter__Blob.cc +++ b/src/foundation_pEp_jniadapter__Blob.cc @@ -1,11 +1,6 @@ -#include #include #include #include -#include -#include -#include - #include "jniutils.hh" #include "throw_pEp_exception.hh" #include "foundation_pEp_jniadapter__Blob.h" diff --git a/src/gen_cpp_Engine.ysl2 b/src/gen_cpp_Engine.ysl2 index 4881b7b..853d4b0 100644 --- a/src/gen_cpp_Engine.ysl2 +++ b/src/gen_cpp_Engine.ysl2 @@ -9,14 +9,11 @@ tstylesheet { template "interface" document("foundation_pEp_jniadapter_{@name}.cc", "text") || - #include - #include #include #include #include #include - #include - #include "foundation_pEp_jniadapter_«@name».h" + #include "foundation_pEp_jniadapter_Engine.h" #include "throw_pEp_exception.hh" #include "jniutils.hh" @@ -50,7 +47,9 @@ tstylesheet { jobject obj`apply "parm[in|inout]", mode=sig` ) { - pEpLog("called"); + pEpLog("called with lock_guard"); + std::lock_guard l(*get_engine_java_object_mutex(env, obj)); + || apply "parm[in|inout]", mode=in; diff --git a/src/gen_cpp_Message.ysl2 b/src/gen_cpp_Message.ysl2 index d73497a..2d301e7 100644 --- a/src/gen_cpp_Message.ysl2 +++ b/src/gen_cpp_Message.ysl2 @@ -12,15 +12,8 @@ tstylesheet { document("foundation_pEp_jniadapter_{$jname}.cc", "text") { || - #include - #include - #include - #include - #include - #include #include #include - #include "jniutils.hh" #include "throw_pEp_exception.hh" #include "foundation_pEp_jniadapter_«$jname».h" diff --git a/src/identity_api.cc b/src/identity_api.cc index e11d4ab..feccf11 100644 --- a/src/identity_api.cc +++ b/src/identity_api.cc @@ -1,7 +1,4 @@ -#include #include -#include "jniutils.hh" - extern "C" { diff --git a/src/jniutils.cc b/src/jniutils.cc index f2d67ed..654ba8b 100644 --- a/src/jniutils.cc +++ b/src/jniutils.cc @@ -1,11 +1,5 @@ #include "jniutils.hh" -#include -#include -#include -#include -#include -#include - +#include #ifndef __LP64__ #include #define time_t time64_t @@ -17,6 +11,53 @@ namespace pEp { namespace JNIAdapter { + std::mutex global_mutex; + std::unordered_map engine_objhash_mutex; + + std::mutex* get_engine_java_object_mutex( + JNIEnv *env, + jobject obj + ) + { + int engine_obj_hash = (int)callIntMethod(env, obj, "hashCode"); + assert(engine_obj_hash); + std::mutex *engine_obj_mutex = engine_objhash_mutex.at(engine_obj_hash); + pEpLog(engine_obj_mutex << " with native_handle: " << engine_obj_mutex->native_handle() << " for java object hash: " << engine_obj_hash); + assert(engine_obj_mutex); + return engine_obj_mutex; + } + + void create_engine_java_object_mutex( + JNIEnv *env, + jobject obj + ) + { + int engine_obj_hash = (int)callIntMethod(env, obj, "hashCode"); + assert(engine_obj_hash); + std::mutex *engine_obj_mutex = new std::mutex(); + pEpLog(engine_obj_mutex << " with native_handle: " << engine_obj_mutex->native_handle() << " for java object hash: " << engine_obj_hash); + assert(engine_obj_mutex); + if(get_engine_java_object_mutex(env, obj) != nullptr ) { + pEpLog("Fatal: mutex already existing for this object"); + assert(0); + } + engine_objhash_mutex.insert(std::make_pair(engine_obj_hash, engine_obj_mutex )); + } + + void release_engine_java_object_mutex( + JNIEnv *env, + jobject obj + ) + { + int engine_obj_hash = (int)callIntMethod(env, obj, "hashCode"); + assert(engine_obj_hash); + std::mutex *engine_obj_mutex = engine_objhash_mutex.at(engine_obj_hash); + pEpLog(engine_obj_mutex << " with native_handle: " << engine_obj_mutex->native_handle() << " for java object hash: " << engine_obj_hash); + assert(engine_obj_mutex); + engine_objhash_mutex.erase(engine_obj_hash); + delete engine_obj_mutex; + } + jclass findClass(JNIEnv *env, const char *classname) { jclass clazz = env->FindClass(classname); diff --git a/src/jniutils.hh b/src/jniutils.hh index 88597de..57a7232 100644 --- a/src/jniutils.hh +++ b/src/jniutils.hh @@ -1,8 +1,6 @@ #pragma once - -#include -#include -#include +#include +#include #include #include #include @@ -15,11 +13,42 @@ #define LOG_TAG "pEpJNIAdapter" #define LOGD(...) __android_log_print(ANDROID_LOG_ERROR,LOG_TAG,__VA_ARGS__) #else -#define LOGD(...) +#define LOGD(...) do{}while(0) #endif namespace pEp { namespace JNIAdapter { + + // Global mutex needs to be locked in all constructors which insert their own mutex object + // into the unordered_map (which is thread safe for read, but not for write) + extern std::mutex global_mutex; + + // Stores mutex per java object + extern std::unordered_map engine_objhash_mutex; + + // needs to be called after create_engine_java_object_mutex() + // and before release_engine_java_object_mutex() + // Thread safe + std::mutex* get_engine_java_object_mutex( + JNIEnv *env, + jobject me + ); + + // Needs to be called exactly once per obj, in the constructor of the obj + // You need to lock a global mutex before calling this function (write to unordered_map) + void create_engine_java_object_mutex( + JNIEnv *env, + jobject me + ); + + // Needs to be called exactly once per obj, in the destructor of this obj + // You need to lock a global mutex before calling this function (write to unordered_map) + void release_engine_java_object_mutex( + JNIEnv *env, + jobject me + ); + + jclass findClass(JNIEnv *env, const char *classname); jfieldID getFieldID( diff --git a/test/java/foundation/pEp/jniadapter/test/jni88/TestMain.java b/test/java/foundation/pEp/jniadapter/test/jni88/TestMain.java index 4b27f39..bc53fb5 100644 --- a/test/java/foundation/pEp/jniadapter/test/jni88/TestMain.java +++ b/test/java/foundation/pEp/jniadapter/test/jni88/TestMain.java @@ -89,8 +89,8 @@ class TestThread extends Thread { class TestMain { public static void main(String[] args) { // Test parameters - boolean useSharedEngine = true; - int numThreads = 2; + boolean useSharedEngine = false; + int numThreads = 200; int numIters = 1000000000; Engine sharedEngine = null; diff --git a/test/java/foundation/pEp/jniadapter/test/jni92/TestMain.java b/test/java/foundation/pEp/jniadapter/test/jni92/TestMain.java index aaff4e9..16a020a 100644 --- a/test/java/foundation/pEp/jniadapter/test/jni92/TestMain.java +++ b/test/java/foundation/pEp/jniadapter/test/jni92/TestMain.java @@ -4,6 +4,7 @@ import foundation.pEp.jniadapter.*; import java.lang.Thread; import java.util.Vector; +import java.util.function.Consumer; /* @@ -14,13 +15,15 @@ https://pep.foundation/jira/browse/JNI-81 */ class TestThread extends Thread { - TestThread(String threadName) { + int nrEngines = 1; + TestThread(String threadName, int nrEngines) { Thread.currentThread().setName(threadName); + this.nrEngines = nrEngines; } public void run() { TestUtils.logH1( "Thread Starting"); - TestMain.TestMainRun(2); + TestMain.TestMainRun(nrEngines); } } @@ -31,7 +34,7 @@ class TestMain { Engine e; TestUtils.logH2("Creating new Engine"); e = new Engine(); - TestUtils.log("Engine created\n"); + TestUtils.log("Engine created with java object hash: " + e.hashCode()); return e; } @@ -43,51 +46,64 @@ class TestMain { return ev; } - public static void own_identities_retrieve_on_EngineVector(Vector ev) { + public static void engineConsumer(Vector ev, Consumer ec) { ev.forEach(e -> { - TestUtils.logH2("own_identities_retrieve()"); - e.own_identities_retrieve(); - TestUtils.log("\n"); + TestUtils.logH2("engineConsumer: on engine object hash: " + e.hashCode()); + ec.accept(e); }); } - public static void TestMainRun(int nrEngines) { Vector engineVector = TestMain.createEngines(nrEngines); // TestUtils.sleep(200); - TestMain.own_identities_retrieve_on_EngineVector(engineVector); + Consumer c = (e) -> { +// Vector v = e.own_identities_retrieve(); +// +// TestUtils.log("own idents: " + v.size()); +// v.forEach( i -> { +// TestUtils.log(TestUtils.identityToString(i)); +// }); + e.getVersion(); +// e.OpenPGP_list_keyinfo(""); + }; +// TestMain.engineConsumer(engineVector, c); } - public static void main(String[] args) throws Exception { + public static void main(String[] args) { TestUtils.logH1("JNI-92 Starting"); + + int nrTestruns = 1000; boolean multiThreaded = true; - int nrEngines = 3; - - if (!multiThreaded) { - // Single Threaded - TestMainRun(nrEngines); - } else { - // Mutli Threaded - Vector tts = new Vector(); - int nrThreads = nrEngines; - for (int i = 0; i < nrThreads; i++) { - tts.add(new TestThread("TestThread-" + i)); + int nrThreads = 200; + int nrEnginesPerThread = 1000; + + for (int run = 0; run < nrTestruns; run++ ) { + TestUtils.logH1("Testrun Nr: " + run); + if (!multiThreaded) { + // Single Threaded + TestMainRun(nrEnginesPerThread); + } else { + // Mutli Threaded + Vector tts = new Vector(); + for (int i = 0; i < nrThreads; i++) { + tts.add(new TestThread("TestThread-" + i, nrEnginesPerThread)); // TestUtils.sleep(200); - } + } - tts.forEach(t -> { - t.start(); + tts.forEach(t -> { + t.start(); // TestUtils.sleep(2000); - }); - - tts.forEach(t -> { - try { - t.join(); - } catch (Exception e) { - TestUtils.log("Exception joining thread" + e.toString()); - } - }); + }); + + tts.forEach(t -> { + try { + t.join(); + } catch (Exception e) { + TestUtils.log("Exception joining thread" + e.toString()); + } + }); + } } } }