RUM-1330 Make sure we use try-locks in our NDK signal catcher by mariusc83 · Pull Request #1665 · DataDog/dd-sdk-android

mariusc83 · 2023-10-10T15:08:10Z

What does this PR do?

We recently had some reported issues in the flutter sdk regarding some ANRs caused by our signal catcher. Basically what was happening is that our code inside the signal catcher was trying to acquire a mutex lock which was already acquired by another thread (we could not really reproduce this). We chose to go with the safest mechanism here in order to avoid any other issues and use try locks instead of simple locks.

DataDog/dd-sdk-flutter#487

Motivation

What inspired you to submit this pull request?

Additional Notes

Anything else we should know when reviewing?

Review checklist (to be filled by reviewers)

Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
Make sure you discussed the feature or bugfix with the maintaining team in an Issue
Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

mariusc83 · 2023-10-10T15:26:09Z

 #include "utils/string-utils.h"

+
+#ifndef NDEBUG


I actually don't think we need this directive check here but just in case

We probably do, in fact the whole test would probably need to be NDEBUG specific

fuzzybinary · 2023-10-10T15:35:42Z


 static const char *LOG_TAG = "DatadogNdkCrashReporter";
+
+#ifndef NDEBUG


I feel like this is a lot of changes just to support the test. Maybe it makes sense to leave the mutex as static and instead create an NDEBUG wrapped function just to lock it.

fuzzybinary · 2023-10-10T15:36:58Z


    // sync everything
-    pthread_mutex_lock(&handler_mutex);
    if (tracking_consent != tracking_consent_granted) {


Is there any reason we can't move this check up before the mutex lock? I wouldn't think we'd care about a race condition here.

it is moved before, check in the modified file

fuzzybinary · 2023-10-10T15:37:46Z

 #include "utils/string-utils.h"

+
+#ifndef NDEBUG


We probably do, in fact the whole test would probably need to be NDEBUG specific

codecov-commenter · 2023-10-10T15:43:05Z

Codecov Report

Merging #1665 (1737ba4) into develop (a5d678d) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #1665      +/-   ##
===========================================
- Coverage    83.73%   83.72%   -0.01%     
===========================================
  Files          459      459              
  Lines        15778    15778              
  Branches      2354     2354              
===========================================
- Hits         13211    13209       -2     
- Misses        1938     1939       +1     
- Partials       629      630       +1

see 16 files with indirect coverage changes

0xnm · 2023-10-11T07:23:20Z

-        pthread_mutex_unlock(&handler_mutex);
+        return;
+    }
+    if(pthread_mutex_trylock(&handler_mutex)!=0){


Suggested change

if(pthread_mutex_trylock(&handler_mutex)!=0){

if(pthread_mutex_trylock(&handler_mutex) != 0){

I usually apply ktlintFormat at the end of my PR but of course for C code doesn't work :)

0xnm · 2023-10-11T07:23:30Z

                         jstring storage_path) {
    using namespace stringutils;
-    pthread_mutex_lock(&handler_mutex);
+    if(pthread_mutex_trylock(&handler_mutex)!=0){


Suggested change

if(pthread_mutex_trylock(&handler_mutex)!=0){

if(pthread_mutex_trylock(&handler_mutex) != 0){

Warning
According to the documentation for pthread_mutex_trylock, there might be several return value. Maybe it'd be relevant to actually look at the return value and act accordingly.

If successful, the pthread_mutex_lock() and pthread_mutex_unlock() functions shall return zero; otherwise, an error number shall be returned to indicate the error.

0xnm · 2023-10-11T07:23:59Z

    // It may happen that the process is killed during this function execution.
    // Therefore this function may never return.
-    pthread_mutex_lock(&mutex);
+    if(pthread_mutex_trylock(&mutex)!=0){


Suggested change

if(pthread_mutex_trylock(&mutex)!=0){

if(pthread_mutex_trylock(&mutex) != 0){

xgouchet · 2023-10-11T08:05:01Z

    fun mustWriteAnErrorLog_whenHandlingSignal_whenConsentUpdatedToGranted() {
        val fakeSignal = forge.anInt(min = 1, max = 32)
-        // we need to keep this this size because we are using a buffer of [30] size in c++ for
+        // we need to keep this size because we are using a buffer of [30] size in c++ for


Note
You're talking about the (size = 20) below right? Wouldn't it be better to have a constant const val SIGNAL_SIZE = 20 (declared in the companion object for example) and reuse it everywhere?
Or because we know the size can be at most 30, user something like val signalSize = forge.anInt(1, 30)?

I actually need to check this but I think this is not needed anymore and I need to just forge a positive int there as the serialization was changed

xgouchet · 2023-10-11T08:15:39Z

                         jstring storage_path) {
    using namespace stringutils;
-    pthread_mutex_lock(&handler_mutex);
+    if(pthread_mutex_trylock(&handler_mutex)!=0){


Warning
According to the documentation for pthread_mutex_trylock, there might be several return value. Maybe it'd be relevant to actually look at the return value and act accordingly.

If successful, the pthread_mutex_lock() and pthread_mutex_unlock() functions shall return zero; otherwise, an error number shall be returned to indicate the error.

mariusc83 · 2023-10-11T08:22:21Z

@xgouchet cannot reply on those comments for some reason so I will reply here, I've read the docs before adding this logic and actually we do not want to do anything in case we cannot acquire the lock, we just do not write the error log as there is no other action that we can take in that case. We could not find out who's acquiring that lock in the tests we performed (me and @fuzzybinary) and we do not want to block the process in the signal catcher in this case.

xgouchet · 2023-10-11T08:36:26Z

@mariusc83 in that case, then maybe add a comment explaining that 👍

xgouchet · 2023-10-11T11:35:17Z

-        val errorMessage = forge.anAlphabeticalString()
-        val errorStack = forge.anAlphabeticalString()
+        val fakeSignal = forge.aPositiveInt(true)
+        val fakeSignalName = forge.anAlphabeticalString(size = 20)


Note
Can you use a more random thing than 20, or at least use a constant explaining the 30 characters constraint

We actually do not need these limitations anymore as this was needed before when that part of code was written in pure C and we used fixed length memory allocation for that. Currently that code is C++ and allows dynamic allocation.

mariusc83 force-pushed the mconstantin/rum-1330/use-try-catch-lock-in-ndk-signal-catcher-logic branch from 70e0fc3 to 35753fd Compare October 10, 2023 15:24

mariusc83 marked this pull request as ready for review October 10, 2023 15:24

mariusc83 requested a review from a team as a code owner October 10, 2023 15:24

mariusc83 commented Oct 10, 2023

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-1330/use-try-catch-lock-in-ndk-signal-catcher-logic branch from 35753fd to e21c2a6 Compare October 10, 2023 15:32

fuzzybinary reviewed Oct 10, 2023

View reviewed changes

0xnm previously approved these changes Oct 11, 2023

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-1330/use-try-catch-lock-in-ndk-signal-catcher-logic branch from e21c2a6 to 8c29b47 Compare October 11, 2023 07:47

xgouchet suggested changes Oct 11, 2023

View reviewed changes

mariusc83 dismissed 0xnm’s stale review via 717057a October 11, 2023 08:49

mariusc83 force-pushed the mconstantin/rum-1330/use-try-catch-lock-in-ndk-signal-catcher-logic branch 2 times, most recently from 717057a to e510444 Compare October 11, 2023 08:55

mariusc83 requested review from 0xnm, fuzzybinary and xgouchet October 11, 2023 09:50

xgouchet reviewed Oct 11, 2023

View reviewed changes

RUM-1330 Make sure we use try-locks in our NDK signal catcher

1737ba4

mariusc83 force-pushed the mconstantin/rum-1330/use-try-catch-lock-in-ndk-signal-catcher-logic branch from e510444 to 1737ba4 Compare October 11, 2023 13:28

mariusc83 requested a review from xgouchet October 11, 2023 13:29

xgouchet approved these changes Oct 11, 2023

View reviewed changes

fuzzybinary approved these changes Oct 11, 2023

View reviewed changes

mariusc83 merged commit 1fc5582 into develop Oct 12, 2023

mariusc83 deleted the mconstantin/rum-1330/use-try-catch-lock-in-ndk-signal-catcher-logic branch October 12, 2023 07:14

xgouchet added this to the 2.3.0 milestone Dec 13, 2023


		static const char *LOG_TAG = "DatadogNdkCrashReporter";

		#ifndef NDEBUG

	if(pthread_mutex_trylock(&handler_mutex)!=0){
	if(pthread_mutex_trylock(&handler_mutex) != 0){

	if(pthread_mutex_trylock(&mutex)!=0){
	if(pthread_mutex_trylock(&mutex) != 0){

Uh oh!

Conversation

mariusc83 commented Oct 10, 2023

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Oct 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mariusc83 commented Oct 11, 2023

Uh oh!

xgouchet commented Oct 11, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Oct 10, 2023 •

edited

Loading