You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
[Android] Expose IDataViewer as public Java API - #1534
Expose the native 1DS IDataViewer extension point through the Android Java/JNI API so Android consumers can register a product-owned viewer without changing the bundled DefaultDataViewer implementation.
This enables consumers such as Teams Android to keep broad Network Security Configuration cleartext traffic disabled while owning any product-specific diagnostic transport outside the SDK.
Changes
Add the public Android IDataViewer callback contract.
Add registerDataViewer and unregisterDataViewer to ILogManager and LogManagerProvider.LogManagerImpl.
Add a JNI-backed C++ IDataViewer proxy that:
retains the Java implementation with a global reference;
attaches native SDK worker threads to the JVM when required;
forwards encoded packet payloads as byte[];
caches a stable viewer name;
isolates and clears Java callback exceptions; and
releases JNI references deterministically.
Retain registered proxies per native LogManager and clean them up on unregister/close.
Reject invalid, empty, duplicate, or unknown viewer registrations with explicit failure results.
Add consumer R8/ProGuard rules for reverse-JNI callbacks.
Add Android instrumentation coverage for registration, duplicate rejection, callback dispatch, callback exception isolation, and unregistration.
Scope
This is an additive Android API bridge over the existing native extension point. It does not change:
production telemetry upload or routing;
endpoint selection or authentication;
the existing DefaultDataViewer APIs; or
product-specific transport, endpoint validation, queueing, retry, or feature-gating policy.
Validation
:maesdk:assembleDebug
:app:compileDebugAndroidTestJavaWithJavac
Native Android builds for arm64-v8a, armeabi-v7a, x86, and x86_64
The instrumentation test was compiled but not executed because no Android device was connected.
I reviewed whether this API is necessary and proportionate. The capability is justified, but the PR should not merge unchanged.
Android currently has no Java mechanism for registering a product-owned implementation of the native IDataViewer extension point. The existing DefaultDataViewer path owns its HTTP transport and requires a cleartext http:// private-subnet endpoint, so it does not address the stated product need without broader cleartext configuration.
The JNI bridge is therefore a reasonable direction, but the existing inline findings are merge blockers:
Adding abstract methods to public ILogManager creates an avoidable source-compatibility break; use source-compatible defaults or a separate extension interface.
Reentrant close() from receiveData() can remove viewers while native dispatch is iterating the collection.
The instrumentation test must verify that callbacks actually stop after unregistering and should be executed on a device/emulator.
With those addressed, the scope is proportionate to the demonstrated gap.
Preserves source compatibility, fixes reentrant dispatch, and strengthens
the unregistration test.
registerDataViewer and unregisterDataViewer become default methods on
ILogManager returning false. Adding abstract methods to a public
interface would break every consumer-owned implementation and test double
on upgrade, despite the change being additive in intent. LogManagerImpl
overrides both, so the native path is unaffected.
DispatchDataViewerEvent now iterates a snapshot of the viewer collection
rather than the member vector. m_dataViewerMapLock is recursive, so a
viewer that reenters the SDK from ReceiveData - closing the owning
LogManager, which unregisters every viewer - was admitted back in and
erased the vector while dispatch was still walking it, invalidating the
iterator. Exposing IDataViewer to arbitrary Java implementations makes
that reachable from outside the SDK, so the hazard is fixed rather than
only documented. Holding shared_ptr copies also keeps each viewer alive
across its own callback. The IDataViewer contract now prohibits closing
the owning manager from a callback, and a unit test covers a viewer that
unregisters everything from ReceiveData.
The instrumentation test asserted only the native return value of
unregisterDataViewer, so a bridge that dropped its bookkeeping entry but
left the proxy registered in DataViewerCollection would have passed. It
now drives a second dispatch after unregistering, using the still
registered throwing viewer as the witness that a dispatch really
occurred, and asserts the unregistered viewer's callback count does not
increase.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
IDataViewer returns the endpoint by const reference, so the referent must
outlive the call and must not be mutated while a caller holds it. The
proxy updated a shared member under a mutex and then returned a reference
to it, releasing the lock on return: two concurrent callers could read and
write the same string at once, so the mutex gave no protection.
Use a thread_local buffer instead, which gives each calling thread its own
storage and removes the need for the lock. Behaviour is unchanged: the
endpoint is still read from Java on every call, so a viewer that changes
endpoints still reports the current one.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
DispatchDataViewerEvent held m_dataViewerMapLock for the whole callback
loop, so the SDK ran arbitrary viewer code while holding one of its own
locks. Registration acquires the JNI viewer mutex and then this lock,
while a callback that reenters registration waits for the JNI mutex with
this lock already held, so the two orders could deadlock: the collection
lock is recursive, but that only helps the thread already holding it, not
the one blocked behind it.
Reentrancy is not required for this to hurt. Because the lock spanned
every callback, any registration, unregistration or LogManager close on
any thread blocked until all callbacks returned. On Android the callback
body is a socket write, so closing the manager could stall behind a slow
or half-open viewer connection.
Take the snapshot under the lock, release it, then dispatch outside it.
The shared_ptr copies still keep each viewer alive for the duration of
its own callback, and viewers that were registered when dispatch began
still receive the in-flight packet.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
NewByteArray reports failure by returning null and leaving an
OutOfMemoryError pending, but the guard tested the null first. Because ||
short circuits, ClearPendingException was skipped on exactly the path that
needed it, so the exception stayed pending and escaped the callback.
On a thread the proxy attached itself that only means detaching with an
exception pending. On an already attached thread nothing clears it, and
every later JNI call on that thread is undefined: the dispatch loop moves
straight to the next viewer and calls NewByteArray again, which CheckJNI
reports as a fatal error. With a single viewer the exception instead
surfaces at an unrelated Java frame, so one viewer running out of memory
is no longer isolated from the SDK or the app.
Evaluate ClearPendingException first so it always runs, which also matches
ReadString, and release the array when an exception was already pending but
the allocation succeeded, so that branch no longer leaks a local reference.
The successful path is unchanged: it already evaluated both operands.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
liblog is only linked on Android when internal logging is enabled
(lib/CMakeLists.txt), so the unconditional __android_log_print calls added
for the Java IDataViewer support broke MATSDK_DISABLE_LOGGING=ON builds with
undefined references at link time.
Wrap the android/log.h includes and every new call site in
#ifdef HAVE_MAT_LOGGING, matching the convention already used throughout
LogManager_jni.cpp, and void the parameters that are only read by the log
statements so the logging-disabled build stays warning free.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Thanks for the review — that framing of the gap matches my intent, and I have gone through the three blockers. Status:
1. Source compatibility on ILogManager — addressed in 65d01fa7.registerDataViewer and unregisterDataViewer are now default methods returning false, so existing implementations and test doubles keep compiling and only viewer-aware implementations override them.
2. Reentrant close() during native dispatch — addressed in a3604e4a, plus documentation in 65d01fa7.DispatchDataViewerEvent now copies the viewer snapshot inside a nested lock scope and runs the dispatch loop with m_dataViewerMapLock released, so a reentrant close can no longer mutate the collection while it is being iterated. That change also removed an AB-BA deadlock against javaDataViewersMutex that Copilot spotted separately. The IDataViewer javadoc still prohibits reentering the SDK from a callback, including closing the owning manager, since close unregisters every viewer while a callback may be in flight.
Two further fixes landed on top: d42ddb30 clears the pending JNI exception when the packet allocation fails, and dbb9728b guards the new __android_log_print calls behind HAVE_MAT_LOGGING so the MATSDK_DISABLE_LOGGING=ON build links again.
3. Instrumentation test — assertions strengthened, and the defect they exposed is fixed in c7ad116f. The test now drives a second dispatch after unregistering and asserts the unregistered viewer's callback count does not move, using the still-registered viewer as a witness that a dispatch really occurred.
Writing that assertion surfaced a real product defect rather than a test problem, so c7ad116f fixes it. DataViewerCollection::UnregisterViewer compared its two const char* operands with == — addresses, not characters — while the sibling GetViewerFromCollection uses strcmp. Unregistration therefore only matched when the caller passed back the exact pointer the viewer returns from GetName(). Every pre-existing caller did precisely that, so the bug stayed latent; this PR adds the first callers that cannot, because the name arrives from a Java string. The effect was that unregisterDataViewer always threw internally and returned false, leaving the proxy registered and still receiving packets after unregister or close — exactly the failure mode your third point is meant to catch.
RegisterViewer already rejects duplicates by value through that same strcmp lookup, so no two registered viewers can share a name and the change simply makes unregistration symmetric with the check that guards registration. Existing callers are unaffected, since identical pointers necessarily have identical contents. I added a unit test that unregisters through a separately allocated name, which is the case the existing coverage could not distinguish.
Flagging that one explicitly because it touches pre-existing code in lib/api rather than anything this PR introduced — happy to split it into its own PR if you would prefer it reviewed separately, though the instrumentation test cannot pass without it.
Remaining on my side is the device run of the instrumentation suite, which I will report back here.
UnregisterViewer compared the two const char* operands with ==, which
compares addresses rather than characters, while the sibling lookup in
GetViewerFromCollection uses strcmp. Unregistration therefore only matched
when the caller passed back the exact pointer the viewer returns from
GetName().
Every pre-existing caller did exactly that, so the defect stayed latent. The
Java IDataViewer bridge adds the first callers that build the name
independently of the viewer object - it arrives as a Java string - so the
lookup fails, UnregisterViewer throws, and the proxy is left registered and
still receiving packets after unregisterDataViewer or close.
RegisterViewer already rejects duplicates by value through the same strcmp
lookup, so no two registered viewers can share a name and this makes
unregistration symmetric with the check that guards registration. Existing
callers are unaffected: identical pointers necessarily have identical
contents.
Also include <cstring> explicitly rather than relying on a transitive
include for the strcmp that this file already used, and cover the case with
a unit test that unregisters through a separately allocated name.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
IsViewerEnabled() called IsTransmissionEnabled() on every viewer while
holding m_dataViewerMapLock. For a Java viewer that is a JNI call into
the JVM, so the native to Java edge that the earlier dispatch fix
removed from ReceiveData still existed here: registration takes the JNI
viewer mutex and then this lock, so the cycle survived, and a slow Java
callback still stalled registration, unregistration and LogManager
close. IsViewerEnabled() is public on IDataViewerCollection, so fixing
only the internal caller would have left the cycle open to external
callers.
Both functions now snapshot the collection under the lock and evaluate
the predicate after releasing it. DispatchDataViewerEvent reuses its
existing snapshot instead of calling IsViewerEnabled() first, which also
removes a second lock acquisition: the set of viewers the enabled
decision was made about was not necessarily the set that was dispatched
to.
std::any_of on an empty range returns false, matching the previous
empty() plus find_if formulation.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This describes a per-viewer callback gate, but DataViewerCollection::DispatchDataViewerEvent checks whether any viewer is enabled and then invokes every viewer (lib/api/DataViewerCollection.cpp:38-46). A viewer returning false can therefore still receive packets whenever another viewer returns true. Please document this collection-wide gate so Java implementations do not treat false as protection from callbacks.
bmehta001 Following up on the verification you asked for - both suites have now been run at head (7682ae21), not just reasoned about.
Host unit tests - Solutions\MSTelemetrySDK.sln, Tests\UnitTests, x64 Debug, WinHTTP:
1298 tests from 99 test suites ran. (101509 ms total)
[ PASSED ] 1298 tests.
Full suite, zero failures. DataViewerCollectionTests is 24/24, including the two new cases.
I also ran a negative control on the strcmp fix, because a regression test that has never failed is not worth much. Reverting just UnregisterViewer to == and rebuilding:
[ OK ] UnregisterViewer_ViewerNameIsNullPtr_ThrowsInvalidArgumentException
[ OK ] UnregisterViewer_ViewerNameIsNotRegistered_ThrowsInvalidArgumentException
[ OK ] UnregisterViewer_ViewerNameIsRegistered_UnregistersCorrectly
[ FAILED ] UnregisterViewer_ViewerNameMatchesByValue_UnregistersCorrectly
Expected: UnregisterViewer(equalName.c_str()) doesn't throw an exception.
Actual: it throws std::invalid_argument with description
"Viewer: 'sharedName' is not currently registered"
The three pre-existing tests still pass against the broken comparison, which is exactly why this defect survived since 2019 - they all hand back viewer->GetName(), so pointer equality holds by accident. The new test is the only one that discriminates.
Android instrumentation tests - LogManagerDDVUnitTest on an API 36 x86_64 emulator, via :app:connectedDebugAndroidTest:
registerDataViewer_whenCallbackThrows_continuesDispatchAndStopsAfterUnregister is the case that exercises the JNI unregister path end to end - it is the one that would have failed on the == comparison, since it unregisters via a freshly converted name.c_str().
One correction to my earlier reply. In the thread on DataViewerCollection.cpp I said the lock is released before viewer callbacks. That was only true of ReceiveData; IsViewerEnabled() was still calling IsTransmissionEnabled() - a JNI call for Java viewers - while holding m_dataViewerMapLock, so the lock cycle survived. Copilot caught it and it is fixed in 7682ae21: both IsViewerEnabled() and DispatchDataViewerEvent() now snapshot under the lock and evaluate after releasing it, and dispatch reuses its single snapshot instead of locking a second time. The statement is accurate as of that commit.
All review threads are now resolved. Happy to run anything else you would like to see before you re-review.
DispatchDataViewerEvent checked whether any viewer was enabled and then
called ReceiveData on every viewer. DefaultDataViewer re-checks
IsTransmissionEnabled() at the top of its own ReceiveData, so the
collection-wide test was only a short-circuit and a disabled C++ viewer
discarded the packet itself. JavaDataViewerProxy has no equivalent
guard, so a disabled Java viewer was handed encoded telemetry whenever
any other viewer was enabled, contradicting the documented contract of
IDataViewer.isTransmissionEnabled(). The check now runs per viewer on
the existing snapshot, which replaces the collection-wide scan rather
than adding to it and skips the byte array allocation and JNI call for
viewers that are not accepting data.
nativeFlushAndTeardown only called ILogManager::FlushAndTeardown().
That is terminal - LogManagerImpl sets m_alive to false and GetLogger()
returns nullptr from then on, and nothing sets it back - but it does
not unregister data viewers, so the proxies stayed in the native
collection and in the javaDataViewers map. Their JNI global references
pinned the application's IDataViewer objects, and everything those
referenced, until close() or process exit even though no further
callback could occur. Teardown now releases them too, ordered after the
flush so viewers still observe the final packets.
closeJavaDataViewers clears its bookkeeping under the lock and returns
early once the manager pointer is null, so a later close() is a no-op.
It also nulls that manager pointer, so a post-teardown flush or
getLogger now returns early instead of entering a dead manager.
Tests: mixed enabled/disabled and no-viewer-enabled dispatch cases, a
Java gating test, and a Java test that flushAndTeardown releases a
registered viewer without close(). All four fail without the
corresponding fix.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
DataViewerCollection now releases its dispatch lock before invoking viewers, so concurrent dispatches can call the same Java viewer at the same time. Saying only that callbacks occur on “an SDK worker thread” leaves consumers unaware that their implementation must be thread-safe. Please document the concurrency explicitly (or serialize callbacks per viewer).
Dispatch takes a snapshot and invokes viewers without holding the
collection lock, so unregister and close return without waiting for a
callback already in progress. Record that guarantee and its consequences
on the public surfaces rather than coordinating removal with dispatch,
which would reinstate the lock cycle the snapshot exists to break.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
DispatchDataViewerEvent and IsViewerEnabled are noexcept, and both copy
the viewer collection to avoid invoking viewers under the lock. That copy
allocates, so bad_alloc - or a system_error from the lock - would
terminate the process instead of costing one packet of diagnostic data.
Route both through a guarded helper that reports failure so the caller
skips the dispatch. MATSDK_TRY/MATSDK_CATCH degrade to if(true)/if(false)
where exceptions are disabled, so the build without C++ exceptions is
unaffected.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The snapshot dispatch is what lets UnregisterViewer return while a viewer
callback is still running, and IDataViewer now documents that guarantee.
Pin it down so a later change cannot quietly reintroduce the wait, and
with it the deadlock the snapshot exists to prevent.
The viewer parks inside ReceiveData and is still parked when unregister
returns, which distinguishes a genuinely non-blocking unregister from one
that merely raced a callback that had already finished. Unregister runs on
a future with a timeout so a regression fails the test rather than hanging
the run. Verified by reinstating the lock across the callback loop: the
test fails with the intended message instead of deadlocking.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…paths
closeJavaDataViewers() nulls ManagerAndConfig::manager, and flushAndTeardown()
now reaches it as well as close(). nativeGetLogger() dereferenced that pointer
unconditionally, so a getLogger() after either call faulted natively. Capture
the manager under jniManagersMutex and return 0 instead, which surfaces as the
NullPointerException LogManagerImpl.getLogger() already raises for a null handle.
JavaDataViewerProxy::Create() and ReadString() are noexcept but allocated: the
proxy, the shared_ptr control block and the name string could each throw
bad_alloc and terminate. Use nothrow new for the object, since the macros
degrade to if (true)/if (false) when exceptions are disabled and a throwing new
would abort uncatchably, and guard the remaining allocations.
shared_ptr::reset(p) is specified as shared_ptr(p).swap(*this), and that
constructor deletes p if the control block allocation throws. The catch handler
deleted the same pointer a second time. Drop the manual delete: reset() already
owns the failure path, so the guard alone is correct. The nothrow new this
replaces only mattered with exceptions disabled, which this target cannot build
- LogManager_jni.cpp shares the source list and has raw throws.
The privacy guard, signals and sanitizer register/unregister entry points
checked only their helper pointer and dereferenced logManager unguarded, so a
call after close() - and now after flushAndTeardown() - faulted. Check the
manager as the neighbouring entry points in this file already do.
The source was added to lib/CMakeLists.txt but not to Android.bp, and
LogManager_jni.cpp calls JavaDataViewerProxy::Create, so the libmaesdk Soong
target would fail to link with unresolved symbols. The other lib/jni sources
missing from Android.bp are conditional on optional modules in CMake; this one
is in the unconditional list.
closeJavaDataViewers() retired the native manager pointer, and this
branch newly calls it from flushAndTeardown(), so teardown inherited
close()'s handle-retiring semantics. That left the still-open Java
LogManager handing out a SemanticContext wrapping pointer 0 -
SemanticContext_jni dereferences it with no null check in any method -
and made nativeRemoveEventListener skip RemoveEventListener while
erasing its bookkeeping, dropping the last shared_ptr to a listener the
ILogManager still held by raw reference.
Retirement now belongs to close() alone, via a retireManager flag on
closeJavaDataViewers(). It stays inside the javaDataViewersMutex
critical section that hands off the viewer map: nativeRegisterDataViewer
checks manager and inserts under that lock only, so retiring outside it
would let a registration land between the handoff and the retirement,
leaving a viewer registered with no bookkeeping entry that keeps its JNI
global reference and keeps receiving callbacks after close() returned.
Registration captures manager once into a local under that lock, and
LogManagerImpl.getSemanticContext() now rejects a null native pointer
the way getLogger() already does instead of wrapping it.
Covered by flushAndTeardown_withRegisteredDataViewer_keepsManagerApisUsableUntilClose,
which asserts the manager stays usable after teardown and that close()
still retires it.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
flushAndTeardown() releases the registered viewers but deliberately
leaves manager non-null, so that the still-open Java LogManager keeps
working. nativeRegisterDataViewer was using that pointer as its liveness
check, so after the teardown cleanup pass a registration could still
succeed. The viewer would then never be released unless close() was
called, retaining its JNI global reference and the Java object graph
behind it until close() or process exit - which is exactly what
releasing viewers at teardown is meant to avoid - and contradicting the
contract that close() is not needed to release viewers.
Track the terminal state explicitly instead of inferring it from the
manager pointer: ManagerAndConfig gains viewersClosed, closeJavaDataViewers
sets it for both callers, and registration refuses once it is set. It is
set inside the javaDataViewersMutex critical section that hands off the
viewer map, because nativeRegisterDataViewer checks it and inserts under
that lock only, so updating it outside would let a registration land
between the handoff and the update.
Covered by registerDataViewer_afterFlushAndTeardown_isRejected.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The Gradle build publishes maesdk/consumer-rules.pro via consumerProguardFiles,
but the Soong android_library had no equivalent, so Soong consumers got none of
the keep rules. IDataViewer implementations are reached only through JNI
GetMethodID, so R8 in a consuming app was free to rename or strip receiveData
and getName and break viewer registration at runtime.
Add an optimize block with proguard_flags_files and export_proguard_flags_files.
The export flag is what propagates the rules to reverse dependencies; the flags
files alone would apply only to this module and be a no-op for consumers.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
BasicFuncTests.storageFileSizeDoesntExceedConfiguredSize is flaky on the
macOS/iOS simulator: it fails intermittently on main's own commit 88defca
(failed 2026-09-30, passed 2026-10-01 and 2026-10-02 on identical sources).
No source changes; empty commit only to re-run the matrix.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
KartikDhawaniya please ensure that no-exception builds still work for Android. There are some existing issues in main that I will fix, but please ensure that the changes in this PR, like in lib/jni/LogManager_jni.cpp use the existing macros to skip compiling try/catch code blocks for no-exception builds.
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
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.
bmehta001
left a comment
Summary
Expose the native 1DS
IDataViewerextension point through the Android Java/JNI API so Android consumers can register a product-owned viewer without changing the bundledDefaultDataViewerimplementation.This enables consumers such as Teams Android to keep broad Network Security Configuration cleartext traffic disabled while owning any product-specific diagnostic transport outside the SDK.
Changes
IDataViewercallback contract.registerDataViewerandunregisterDataViewertoILogManagerandLogManagerProvider.LogManagerImpl.IDataViewerproxy that:byte[];Scope
This is an additive Android API bridge over the existing native extension point. It does not change:
DefaultDataViewerAPIs; orValidation
:maesdk:assembleDebug:app:compileDebugAndroidTestJavaWithJavacarm64-v8a,armeabi-v7a,x86, andx86_64The instrumentation test was compiled but not executed because no Android device was connected.