diff --git a/lib/CMakeLists.txt b/lib/CMakeLists.txt index e775caf08..d311be424 100644 --- a/lib/CMakeLists.txt +++ b/lib/CMakeLists.txt @@ -60,6 +60,7 @@ endif() if(MATSDK_BUILD_JNI_WRAPPER) list(APPEND SRCS + jni/JavaDataViewerProxy.cpp jni/JniConvertors.cpp jni/LogManager_jni.cpp jni/Logger_jni.cpp diff --git a/lib/android_build/app/src/androidTest/java/com/microsoft/applications/events/maesdktest/LogManagerDDVUnitTest.java b/lib/android_build/app/src/androidTest/java/com/microsoft/applications/events/maesdktest/LogManagerDDVUnitTest.java index 405749318..a9dd729ee 100644 --- a/lib/android_build/app/src/androidTest/java/com/microsoft/applications/events/maesdktest/LogManagerDDVUnitTest.java +++ b/lib/android_build/app/src/androidTest/java/com/microsoft/applications/events/maesdktest/LogManagerDDVUnitTest.java @@ -25,6 +25,7 @@ import com.microsoft.applications.events.DebugEventType; import com.microsoft.applications.events.DiagLevel; import com.microsoft.applications.events.HttpClient; +import com.microsoft.applications.events.IDataViewer; import com.microsoft.applications.events.ILogConfiguration; import com.microsoft.applications.events.ILogManager; import com.microsoft.applications.events.ILogger; @@ -42,7 +43,10 @@ import java.util.SortedMap; import java.util.TreeMap; import java.util.TreeSet; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.FutureTask; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; import org.junit.Test; import org.junit.runner.RunWith; @@ -247,6 +251,119 @@ public void startDDVonLogManager() { LogManager.flushAndTeardown(); } + @Test + public void registerDataViewer_whenCallbackThrows_continuesDispatchAndStopsAfterUnregister() + throws Exception { + System.loadLibrary("maesdk"); + Context appContext = InstrumentationRegistry.getInstrumentation().getTargetContext(); + if (s_client == null) { + s_client = new MockHttpClient(appContext); + } + OfflineRoom.connectContext(appContext); + + final String token = + "0123456789abcdef9123456789abcdef-01234567-0123-0123-0123-0123456789ab-0124"; + final String factoryName = "JavaDataViewer" + System.nanoTime(); + ILogConfiguration custom = LogManager.logConfigurationFactory(); + custom.set(LogConfigurationKey.CFG_STR_PRIMARY_TOKEN, token); + custom.set(LogConfigurationKey.CFG_STR_COLLECTOR_URL, "https://viewer.contoso.com/"); + custom.set(LogConfigurationKey.CFG_STR_FACTORY_NAME, factoryName); + custom.set(LogConfigurationKey.CFG_STR_CACHE_FILE_PATH, factoryName); + + ILogManager manager = LogManagerProvider.createLogManager(custom); + CountDownLatch receivedPacket = new CountDownLatch(1); + AtomicInteger receivedByteCount = new AtomicInteger(); + AtomicInteger receivingViewerCalls = new AtomicInteger(); + AtomicInteger throwingViewerCalls = new AtomicInteger(); + IDataViewer throwingViewer = + new IDataViewer() { + @Override + public void receiveData(byte[] packetData) { + throwingViewerCalls.incrementAndGet(); + throw new IllegalStateException("Expected callback failure"); + } + + @Override + public String getName() { + return "throwing-viewer"; + } + + @Override + public boolean isTransmissionEnabled() { + return true; + } + + @Override + public String getCurrentEndpoint() { + return ""; + } + }; + IDataViewer receivingViewer = + new IDataViewer() { + @Override + public void receiveData(byte[] packetData) { + receivingViewerCalls.incrementAndGet(); + receivedByteCount.set(packetData.length); + receivedPacket.countDown(); + } + + @Override + public String getName() { + return "receiving-viewer"; + } + + @Override + public boolean isTransmissionEnabled() { + return true; + } + + @Override + public String getCurrentEndpoint() { + return "http://127.0.0.1"; + } + }; + + try { + assertThat(manager.registerDataViewer(throwingViewer), is(true)); + assertThat(manager.registerDataViewer(receivingViewer), is(true)); + assertThat(manager.registerDataViewer(receivingViewer), is(false)); + + ILogger logger = manager.getLogger(token, "java-data-viewer-test", ""); + logger.logEvent("javaDataViewerCallback"); + manager.uploadNow(); + + assertThat(receivedPacket.await(5, TimeUnit.SECONDS), is(true)); + assertThat(receivedByteCount.get(), greaterThan(0)); + + assertThat(manager.unregisterDataViewer("receiving-viewer"), is(true)); + assertThat(manager.unregisterDataViewer("receiving-viewer"), is(false)); + + // Unregistering must actually stop callbacks, not merely drop the bookkeeping entry: a + // bridge that left the proxy in the native DataViewerCollection would still pass the + // assertions above. Drive a second dispatch and use the still-registered throwing viewer + // as the witness that one really occurred, then assert the unregistered viewer was not + // called again. + final int receivingCallsAtUnregister = receivingViewerCalls.get(); + final int throwingCallsAtUnregister = throwingViewerCalls.get(); + + logger.logEvent("javaDataViewerCallbackAfterUnregister"); + manager.uploadNow(); + + final long deadline = System.currentTimeMillis() + 10000; + while (throwingViewerCalls.get() <= throwingCallsAtUnregister + && System.currentTimeMillis() < deadline) { + Thread.sleep(50); + } + + assertThat(throwingViewerCalls.get(), greaterThan(throwingCallsAtUnregister)); + assertThat(receivingViewerCalls.get(), is(receivingCallsAtUnregister)); + + assertThat(manager.unregisterDataViewer("throwing-viewer"), is(true)); + } finally { + manager.close(); + } + } + /* Disabling this test since it requires private modules. diff --git a/lib/android_build/maesdk/consumer-rules.pro b/lib/android_build/maesdk/consumer-rules.pro index e69de29bb..09006a474 100644 --- a/lib/android_build/maesdk/consumer-rules.pro +++ b/lib/android_build/maesdk/consumer-rules.pro @@ -0,0 +1,4 @@ +-keep interface com.microsoft.applications.events.IDataViewer { *; } +-keep class * implements com.microsoft.applications.events.IDataViewer { + public *; +} \ No newline at end of file diff --git a/lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/IDataViewer.java b/lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/IDataViewer.java new file mode 100644 index 000000000..36503859a --- /dev/null +++ b/lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/IDataViewer.java @@ -0,0 +1,32 @@ +// +// Copyright (c) Microsoft Corporation. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +// +package com.microsoft.applications.events; + +import androidx.annotation.Keep; + +/** + * Receives copies of packets uploaded by the SDK. + * + *

Implementations must return a stable, unique name for the lifetime of the registration. + * Callbacks can occur on an SDK worker thread and should return promptly. Implementations must not + * reenter the SDK from within a callback: do not register or unregister viewers, and do not close + * the owning {@link ILogManager}, because closing unregisters every viewer while the callback is + * still in progress. + */ +@Keep +public interface IDataViewer { + + /** Receives an encoded telemetry packet after it has been prepared for upload. */ + void receiveData(byte[] packetData); + + /** Returns the stable, unique name used to register this viewer. */ + String getName(); + + /** Returns whether this viewer is currently accepting packet callbacks. */ + boolean isTransmissionEnabled(); + + /** Returns the endpoint currently used by this viewer, or an empty string when disabled. */ + String getCurrentEndpoint(); +} diff --git a/lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/ILogManager.java b/lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/ILogManager.java index 332331cb1..9a90d7eb1 100644 --- a/lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/ILogManager.java +++ b/lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/ILogManager.java @@ -59,6 +59,34 @@ public interface ILogManager extends AutoCloseable { public String getCurrentEndpoint(); + /** + * Registers a caller-provided data viewer with this LogManager. + * + *

This is an optional capability. The default implementation returns {@code false} so that + * existing implementations of this interface remain source compatible; implementations that + * support data viewers override it. + * + * @return {@code true} when the viewer was registered, {@code false} for invalid input, a + * duplicate viewer name, or when the implementation does not support data viewers + */ + default boolean registerDataViewer(IDataViewer dataViewer) { + return false; + } + + /** + * Unregisters a caller-provided data viewer by its unique name. + * + *

This is an optional capability. The default implementation returns {@code false} so that + * existing implementations of this interface remain source compatible; implementations that + * support data viewers override it. + * + * @return {@code true} when the viewer was unregistered, {@code false} when it was not + * registered, or when the implementation does not support data viewers + */ + default boolean unregisterDataViewer(String viewerName) { + return false; + } + public LogSessionData getLogSessionData(); public void setLevelFilter(int defaultLevel, int[] allowedLevels); diff --git a/lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/LogManagerProvider.java b/lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/LogManagerProvider.java index ba5d41e74..777f47d73 100644 --- a/lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/LogManagerProvider.java +++ b/lib/android_build/maesdk/src/main/java/com/microsoft/applications/events/LogManagerProvider.java @@ -226,6 +226,28 @@ public String getCurrentEndpoint() { return nativeGetCurrentEndpoint(nativeLogManager); } + protected native boolean nativeRegisterDataViewer( + long nativeLogManager, IDataViewer dataViewer); + + @Override + public boolean registerDataViewer(IDataViewer dataViewer) { + if (dataViewer == null) { + return false; + } + return nativeRegisterDataViewer(nativeLogManager, dataViewer); + } + + protected native boolean nativeUnregisterDataViewer( + long nativeLogManager, String viewerName); + + @Override + public boolean unregisterDataViewer(String viewerName) { + if (viewerName == null || viewerName.isEmpty()) { + return false; + } + return nativeUnregisterDataViewer(nativeLogManager, viewerName); + } + protected static class LogSessionDataImpl implements LogSessionData { @Keep private long m_first_time; diff --git a/lib/api/DataViewerCollection.cpp b/lib/api/DataViewerCollection.cpp index 6992fee75..9c4a93205 100644 --- a/lib/api/DataViewerCollection.cpp +++ b/lib/api/DataViewerCollection.cpp @@ -16,7 +16,14 @@ namespace MAT_NS_BEGIN { return; LOCKGUARD(m_dataViewerMapLock); - for(const auto& viewer : m_dataViewerCollection) + // Dispatch over a snapshot rather than the member directly. m_dataViewerMapLock is + // recursive, so a viewer that reenters the SDK from ReceiveData - for example by + // closing the owning LogManager, which unregisters every viewer - would otherwise + // erase from the very vector being iterated here and invalidate the iterator. + // Holding shared_ptr copies additionally keeps each viewer alive for the duration of + // its own callback, even if that callback drops the last other reference to it. + const auto viewers = m_dataViewerCollection; + for(const auto& viewer : viewers) { // Task 3568800: Integrate ThreadPool to IDataViewerCollection viewer->ReceiveData(packetData); diff --git a/lib/jni/JavaDataViewerProxy.cpp b/lib/jni/JavaDataViewerProxy.cpp new file mode 100644 index 000000000..2d49d4d31 --- /dev/null +++ b/lib/jni/JavaDataViewerProxy.cpp @@ -0,0 +1,269 @@ +// +// Copyright (c) Microsoft Corporation. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +// +#include "JavaDataViewerProxy.hpp" + +#include +#include +#include + +namespace MAT_NS_BEGIN +{ + namespace + { + constexpr const char* LOG_TAG = "MAE.JavaDataViewer"; + } + + std::shared_ptr JavaDataViewerProxy::Create( + JNIEnv* env, + jobject dataViewer) noexcept + { + if (env == nullptr || dataViewer == nullptr) + { + return nullptr; + } + + auto proxy = std::shared_ptr(new JavaDataViewerProxy()); + if (env->GetJavaVM(&proxy->m_javaVm) != JNI_OK) + { + return nullptr; + } + + auto dataViewerClass = env->GetObjectClass(dataViewer); + if (dataViewerClass == nullptr || env->ExceptionCheck()) + { + env->ExceptionClear(); + return nullptr; + } + + proxy->m_receiveData = env->GetMethodID(dataViewerClass, "receiveData", "([B)V"); + if (proxy->ClearPendingException(env, "receiveData lookup")) + { + env->DeleteLocalRef(dataViewerClass); + return nullptr; + } + proxy->m_getName = env->GetMethodID(dataViewerClass, "getName", "()Ljava/lang/String;"); + if (proxy->ClearPendingException(env, "getName lookup")) + { + env->DeleteLocalRef(dataViewerClass); + return nullptr; + } + proxy->m_isTransmissionEnabled = + env->GetMethodID(dataViewerClass, "isTransmissionEnabled", "()Z"); + if (proxy->ClearPendingException(env, "isTransmissionEnabled lookup")) + { + env->DeleteLocalRef(dataViewerClass); + return nullptr; + } + proxy->m_getCurrentEndpoint = + env->GetMethodID(dataViewerClass, "getCurrentEndpoint", "()Ljava/lang/String;"); + if (proxy->ClearPendingException(env, "getCurrentEndpoint lookup")) + { + env->DeleteLocalRef(dataViewerClass); + return nullptr; + } + env->DeleteLocalRef(dataViewerClass); + + if (proxy->m_receiveData == nullptr || + proxy->m_getName == nullptr || + proxy->m_isTransmissionEnabled == nullptr || + proxy->m_getCurrentEndpoint == nullptr) + { + return nullptr; + } + + proxy->m_dataViewer = env->NewGlobalRef(dataViewer); + if (proxy->m_dataViewer == nullptr || env->ExceptionCheck()) + { + env->ExceptionClear(); + return nullptr; + } + + if (!proxy->ReadString(env, proxy->m_getName, proxy->m_name) || proxy->m_name.empty()) + { + return nullptr; + } + return proxy; + } + + JavaDataViewerProxy::~JavaDataViewerProxy() noexcept + { + if (m_dataViewer == nullptr) + { + return; + } + + bool attached = false; + auto env = GetEnv(attached); + if (env != nullptr) + { + env->DeleteGlobalRef(m_dataViewer); + } + m_dataViewer = nullptr; + DetachIfNeeded(attached); + } + + void JavaDataViewerProxy::ReceiveData(const std::vector& packetData) noexcept + { + if (packetData.size() > static_cast(std::numeric_limits::max())) + { + __android_log_print(ANDROID_LOG_ERROR, LOG_TAG, "Packet is too large for a Java byte array"); + return; + } + + bool attached = false; + auto env = GetEnv(attached); + if (env == nullptr) + { + return; + } + + auto packet = env->NewByteArray(static_cast(packetData.size())); + if (packet == nullptr || ClearPendingException(env, "receiveData allocation")) + { + DetachIfNeeded(attached); + return; + } + if (!packetData.empty()) + { + env->SetByteArrayRegion( + packet, + 0, + static_cast(packetData.size()), + reinterpret_cast(packetData.data())); + } + + if (!ClearPendingException(env, "receiveData copy")) + { + env->CallVoidMethod(m_dataViewer, m_receiveData, packet); + ClearPendingException(env, "receiveData"); + } + env->DeleteLocalRef(packet); + DetachIfNeeded(attached); + } + + const char* JavaDataViewerProxy::GetName() const noexcept + { + return m_name.c_str(); + } + + bool JavaDataViewerProxy::IsTransmissionEnabled() const noexcept + { + bool attached = false; + auto env = GetEnv(attached); + if (env == nullptr) + { + return false; + } + + auto enabled = env->CallBooleanMethod(m_dataViewer, m_isTransmissionEnabled); + if (ClearPendingException(env, "isTransmissionEnabled")) + { + enabled = JNI_FALSE; + } + DetachIfNeeded(attached); + return enabled == JNI_TRUE; + } + + const std::string& JavaDataViewerProxy::GetCurrentEndpoint() const noexcept + { + // IDataViewer returns the endpoint by reference, so the referent has to outlive the + // call and must not be mutated by a concurrent caller. A thread_local buffer gives + // each calling thread its own storage; a shared member guarded by a mutex would not, + // because the lock is released before the caller reads the reference. + static thread_local std::string currentEndpoint; + + bool attached = false; + auto env = GetEnv(attached); + if (env == nullptr) + { + currentEndpoint.clear(); + return currentEndpoint; + } + + std::string endpoint; + if (ReadString(env, m_getCurrentEndpoint, endpoint)) + { + currentEndpoint = std::move(endpoint); + } + else + { + currentEndpoint.clear(); + } + DetachIfNeeded(attached); + return currentEndpoint; + } + + JNIEnv* JavaDataViewerProxy::GetEnv(bool& attached) const noexcept + { + attached = false; + if (m_javaVm == nullptr) + { + return nullptr; + } + + JNIEnv* env = nullptr; + auto result = m_javaVm->GetEnv(reinterpret_cast(&env), JNI_VERSION_1_6); + if (result == JNI_OK) + { + return env; + } + if (result != JNI_EDETACHED || m_javaVm->AttachCurrentThread(&env, nullptr) != JNI_OK) + { + return nullptr; + } + attached = true; + return env; + } + + void JavaDataViewerProxy::DetachIfNeeded(bool attached) const noexcept + { + if (attached && m_javaVm != nullptr) + { + m_javaVm->DetachCurrentThread(); + } + } + + bool JavaDataViewerProxy::ClearPendingException( + JNIEnv* env, + const char* methodName) const noexcept + { + if (!env->ExceptionCheck()) + { + return false; + } + env->ExceptionClear(); + __android_log_print( + ANDROID_LOG_ERROR, + LOG_TAG, + "Java IDataViewer callback failed: %s", + methodName); + return true; + } + + bool JavaDataViewerProxy::ReadString( + JNIEnv* env, + jmethodID method, + std::string& value) const noexcept + { + auto javaValue = static_cast(env->CallObjectMethod(m_dataViewer, method)); + if (ClearPendingException(env, "string callback") || javaValue == nullptr) + { + return false; + } + + auto chars = env->GetStringUTFChars(javaValue, nullptr); + if (chars == nullptr) + { + ClearPendingException(env, "string conversion"); + env->DeleteLocalRef(javaValue); + return false; + } + value.assign(chars); + env->ReleaseStringUTFChars(javaValue, chars); + env->DeleteLocalRef(javaValue); + return !ClearPendingException(env, "string conversion"); + } + +} MAT_NS_END diff --git a/lib/jni/JavaDataViewerProxy.hpp b/lib/jni/JavaDataViewerProxy.hpp new file mode 100644 index 000000000..ed0936120 --- /dev/null +++ b/lib/jni/JavaDataViewerProxy.hpp @@ -0,0 +1,47 @@ +// +// Copyright (c) Microsoft Corporation. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +// +#ifndef JAVADATAVIEWERPROXY_HPP +#define JAVADATAVIEWERPROXY_HPP + +#include "IDataViewer.hpp" + +#include +#include +#include + +namespace MAT_NS_BEGIN +{ + class JavaDataViewerProxy final : public IDataViewer + { + public: + static std::shared_ptr Create(JNIEnv* env, jobject dataViewer) noexcept; + + ~JavaDataViewerProxy() noexcept override; + + void ReceiveData(const std::vector& packetData) noexcept override; + const char* GetName() const noexcept override; + bool IsTransmissionEnabled() const noexcept override; + const std::string& GetCurrentEndpoint() const noexcept override; + + private: + JavaDataViewerProxy() = default; + + JNIEnv* GetEnv(bool& attached) const noexcept; + void DetachIfNeeded(bool attached) const noexcept; + bool ClearPendingException(JNIEnv* env, const char* methodName) const noexcept; + bool ReadString(JNIEnv* env, jmethodID method, std::string& value) const noexcept; + + JavaVM* m_javaVm = nullptr; + jobject m_dataViewer = nullptr; + jmethodID m_receiveData = nullptr; + jmethodID m_getName = nullptr; + jmethodID m_isTransmissionEnabled = nullptr; + jmethodID m_getCurrentEndpoint = nullptr; + std::string m_name; + }; + +} MAT_NS_END + +#endif diff --git a/lib/jni/LogManager_jni.cpp b/lib/jni/LogManager_jni.cpp index 70cb5b1ec..e5fe35351 100644 --- a/lib/jni/LogManager_jni.cpp +++ b/lib/jni/LogManager_jni.cpp @@ -27,6 +27,7 @@ #include #include "callbacks/DebugSourceInternal.hpp" +#include "JavaDataViewerProxy.hpp" #include "JniConvertors.hpp" #include "LogManagerBase.hpp" #include "WrapperLogManager.hpp" @@ -35,6 +36,9 @@ #endif #include "config/RuntimeConfig_Default.hpp" +#include +#include + using namespace MAT; template <> @@ -869,12 +873,16 @@ namespace ILogConfiguration config; ILogManager* manager; std::shared_ptr ddv; + std::mutex javaDataViewersMutex; + std::unordered_map> javaDataViewers; }; #else struct ManagerAndConfig { ILogConfiguration config; ILogManager* manager; + std::mutex javaDataViewersMutex; + std::unordered_map> javaDataViewers; }; #endif @@ -882,6 +890,53 @@ namespace static MCVector jniManagers; static std::mutex jniManagersMutex; + + ManagerAndConfig* getManagerAndConfig(jlong nativeLogManager) + { + std::lock_guard lock(jniManagersMutex); + if (nativeLogManager < 0 || + nativeLogManager >= static_cast(jniManagers.size())) + { + return nullptr; + } + return jniManagers[nativeLogManager].get(); + } + + void closeJavaDataViewers(ManagerAndConfig& managerAndConfig) + { + ILogManager* manager; + std::unordered_map> dataViewers; + { + std::lock_guard lock(managerAndConfig.javaDataViewersMutex); + manager = managerAndConfig.manager; + { + std::lock_guard managersLock(jniManagersMutex); + managerAndConfig.manager = nullptr; + } + dataViewers.swap(managerAndConfig.javaDataViewers); + } + + if (manager == nullptr) + { + return; + } + for (const auto& dataViewer : dataViewers) + { + try + { + manager->GetDataViewerCollection().UnregisterViewer(dataViewer.first.c_str()); + } + catch (const std::exception& exception) + { + __android_log_print( + ANDROID_LOG_WARN, + "MAE.JavaDataViewer", + "Failed to unregister Java IDataViewer '%s': %s", + dataViewer.first.c_str(), + exception.what()); + } + } + } } extern "C" JNIEXPORT jlong JNICALL @@ -979,17 +1034,14 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na jobject /* this */, jlong nativeLogManager) { + auto managerAndConfig = getManagerAndConfig(nativeLogManager); + if (managerAndConfig == nullptr) { - std::lock_guard lock(jniManagersMutex); - if (nativeLogManager < 0 || nativeLogManager >= static_cast(jniManagers.size())) - { - return; - } - // we reset the manager member of the ManagerAndConfig, - // but the ManagerAndConfig itself will survive until - // the static jniManagers array is destroyed. - jniManagers[nativeLogManager]->manager = nullptr; + return; } + + // The ManagerAndConfig survives until the static jniManagers array is destroyed. + closeJavaDataViewers(*managerAndConfig); } extern "C" JNIEXPORT jobject JNICALL @@ -1526,6 +1578,121 @@ Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_na #endif } +extern "C" JNIEXPORT jboolean JNICALL +Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_nativeRegisterDataViewer( + JNIEnv* env, + jobject /* this */, + jlong native_log_manager, + jobject data_viewer) +{ + auto proxy = JavaDataViewerProxy::Create(env, data_viewer); + if (!proxy) + { + return false; + } + + auto manager_and_config = getManagerAndConfig(native_log_manager); + if (manager_and_config == nullptr) + { + return false; + } + + std::lock_guard lock(manager_and_config->javaDataViewersMutex); + if (manager_and_config->manager == nullptr || + manager_and_config->javaDataViewers.find(proxy->GetName()) != + manager_and_config->javaDataViewers.end()) + { + return false; + } + + bool collectionRegistered = false; + try + { + manager_and_config->manager->GetDataViewerCollection().RegisterViewer(proxy); + collectionRegistered = true; + manager_and_config->javaDataViewers.emplace(proxy->GetName(), proxy); + return true; + } + catch (const std::exception& exception) + { + if (collectionRegistered) + { + try + { + manager_and_config->manager->GetDataViewerCollection().UnregisterViewer( + proxy->GetName()); + } + catch (const std::exception& rollbackException) + { + __android_log_print( + ANDROID_LOG_ERROR, + "MAE.JavaDataViewer", + "Failed to roll back Java IDataViewer '%s': %s", + proxy->GetName(), + rollbackException.what()); + } + } + __android_log_print( + ANDROID_LOG_WARN, + "MAE.JavaDataViewer", + "Failed to register Java IDataViewer '%s': %s", + proxy->GetName(), + exception.what()); + return false; + } +} + +extern "C" JNIEXPORT jboolean JNICALL +Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_nativeUnregisterDataViewer( + JNIEnv* env, + jobject /* this */, + jlong native_log_manager, + jstring viewer_name) +{ + std::string name; + if (!TryJStringToStdString(env, viewer_name, name) || name.empty()) + { + return false; + } + + auto manager_and_config = getManagerAndConfig(native_log_manager); + if (manager_and_config == nullptr) + { + return false; + } + + ILogManager* manager; + std::shared_ptr proxy; + { + std::lock_guard lock(manager_and_config->javaDataViewersMutex); + auto viewer = manager_and_config->javaDataViewers.find(name); + if (manager_and_config->manager == nullptr || + viewer == manager_and_config->javaDataViewers.end()) + { + return false; + } + manager = manager_and_config->manager; + proxy = std::move(viewer->second); + manager_and_config->javaDataViewers.erase(viewer); + } + + try + { + manager->GetDataViewerCollection().UnregisterViewer(name.c_str()); + return true; + } + catch (const std::exception& exception) + { + __android_log_print( + ANDROID_LOG_WARN, + "MAE.JavaDataViewer", + "Failed to unregister Java IDataViewer '%s': %s", + name.c_str(), + exception.what()); + return false; + } +} + extern "C" JNIEXPORT void JNICALL Java_com_microsoft_applications_events_LogManagerProvider_00024LogManagerImpl_nativeGetLogSessionData( JNIEnv* env, diff --git a/tests/unittests/DataViewerCollectionTests.cpp b/tests/unittests/DataViewerCollectionTests.cpp index 57b377fc3..33b51916c 100644 --- a/tests/unittests/DataViewerCollectionTests.cpp +++ b/tests/unittests/DataViewerCollectionTests.cpp @@ -264,3 +264,65 @@ TEST(DataViewerCollectionTests, IsViewerEnabledNoParam_MultipleViewersRegistered ASSERT_TRUE(dataViewerCollection.IsViewerEnabled()); } +namespace +{ + // Mirrors a viewer that reenters the SDK from its own callback - for example a Java + // viewer that closes the owning LogManager from receiveData(), which unregisters every + // viewer. m_dataViewerMapLock is recursive, so the reentrant call is admitted while + // dispatch is still walking the collection. + class ReentrantUnregisteringDataViewer : public IDataViewer + { + public: + + ReentrantUnregisteringDataViewer(const char* name, TestDataViewerCollection& collection) : + m_name(name), m_collection(collection) {} + + void ReceiveData(const std::vector&) noexcept override + { + callCount++; + m_collection.UnregisterAllViewers(); + } + + const char* GetName() const noexcept override + { + return m_name; + } + + bool IsTransmissionEnabled() const noexcept override + { + return true; + } + + const std::string& GetCurrentEndpoint() const noexcept override + { + return m_testEndpoint; + } + + int callCount { 0 }; + const char* m_name; + TestDataViewerCollection& m_collection; + const std::string m_testEndpoint { "TestEndpoint" }; + }; +} + +TEST(DataViewerCollectionTests, DispatchDataViewerEvent_ViewerUnregistersAllFromCallback_DispatchCompletesSafely) +{ + TestDataViewerCollection dataViewerCollection { }; + auto reentrantViewer = std::make_shared("ReentrantViewer", dataViewerCollection); + auto secondViewer = std::make_shared("SecondViewer", /*isTransmissionEnabled*/ true); + + dataViewerCollection.RegisterViewer(reentrantViewer); + dataViewerCollection.RegisterViewer(secondViewer); + + const std::vector packetData { 1, 2, 3 }; + + // Dispatching over the member vector directly would erase it mid-iteration here and + // invalidate the iterator; dispatching over a snapshot completes and still delivers the + // in-flight packet to viewers that were registered when dispatch began. + dataViewerCollection.DispatchDataViewerEvent(packetData); + + ASSERT_EQ(reentrantViewer->callCount, 1); + ASSERT_EQ(secondViewer->localPacketData, packetData); + ASSERT_TRUE(dataViewerCollection.GetCollection().empty()); +} +