From f9471d2967996885ca3b8c33147503576d3767b5 Mon Sep 17 00:00:00 2001 From: fractalwrench Date: Fri, 9 Mar 2018 12:56:09 +0000 Subject: [PATCH 1/4] add failing test for before notify modification --- .../android/ConcurrentBeforeNotifyTest.java | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 sdk/src/androidTest/java/com/bugsnag/android/ConcurrentBeforeNotifyTest.java diff --git a/sdk/src/androidTest/java/com/bugsnag/android/ConcurrentBeforeNotifyTest.java b/sdk/src/androidTest/java/com/bugsnag/android/ConcurrentBeforeNotifyTest.java new file mode 100644 index 0000000000..d09b612602 --- /dev/null +++ b/sdk/src/androidTest/java/com/bugsnag/android/ConcurrentBeforeNotifyTest.java @@ -0,0 +1,46 @@ +package com.bugsnag.android; + +import android.support.test.filters.SmallTest; +import android.support.test.runner.AndroidJUnit4; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; + +import java.util.Collection; + +/** + * Ensures that if a {@link BeforeNotify} is added or removed during iteration, a + * {@link java.util.ConcurrentModificationException} is not thrown + */ +@RunWith(AndroidJUnit4.class) +@SmallTest +public class ConcurrentBeforeNotifyTest { + + private Client client; + + @Before + public void setUp() throws Exception { + client = BugsnagTestUtils.generateClient(); + } + + @Test + public void testClientNotifyModification() throws Exception { + final Collection beforeNotifyTasks = client.config.getBeforeNotifyTasks(); + client.beforeNotify(new BeforeNotify() { + @Override + public boolean run(Error error) { + beforeNotifyTasks.clear(); + return true; + } + }); + client.beforeNotify(new BeforeNotify() { + @Override + public boolean run(Error error) { + return true; + } + }); + client.notify(new RuntimeException()); + } + +} From d693c07b899b6fc0737c5059a1e3145f55c12bb6 Mon Sep 17 00:00:00 2001 From: fractalwrench Date: Fri, 9 Mar 2018 13:41:49 +0000 Subject: [PATCH 2/4] add failing tests for breadcrumb concurrent modification --- ...yTest.java => ConcurrentCallbackTest.java} | 27 +++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) rename sdk/src/androidTest/java/com/bugsnag/android/{ConcurrentBeforeNotifyTest.java => ConcurrentCallbackTest.java} (54%) diff --git a/sdk/src/androidTest/java/com/bugsnag/android/ConcurrentBeforeNotifyTest.java b/sdk/src/androidTest/java/com/bugsnag/android/ConcurrentCallbackTest.java similarity index 54% rename from sdk/src/androidTest/java/com/bugsnag/android/ConcurrentBeforeNotifyTest.java rename to sdk/src/androidTest/java/com/bugsnag/android/ConcurrentCallbackTest.java index d09b612602..9e46e8f422 100644 --- a/sdk/src/androidTest/java/com/bugsnag/android/ConcurrentBeforeNotifyTest.java +++ b/sdk/src/androidTest/java/com/bugsnag/android/ConcurrentCallbackTest.java @@ -1,5 +1,6 @@ package com.bugsnag.android; +import android.support.annotation.NonNull; import android.support.test.filters.SmallTest; import android.support.test.runner.AndroidJUnit4; @@ -10,12 +11,12 @@ import java.util.Collection; /** - * Ensures that if a {@link BeforeNotify} is added or removed during iteration, a + * Ensures that if a callback is added or removed during iteration, a * {@link java.util.ConcurrentModificationException} is not thrown */ @RunWith(AndroidJUnit4.class) @SmallTest -public class ConcurrentBeforeNotifyTest { +public class ConcurrentCallbackTest { private Client client; @@ -43,4 +44,26 @@ public boolean run(Error error) { client.notify(new RuntimeException()); } + @Test + public void testClientBreadcrumbModification() throws Exception { + final Collection breadcrumbTasks = client.config.getBeforeRecordBreadcrumbTasks(); + + client.beforeRecordBreadcrumb(new BeforeRecordBreadcrumb() { + @Override + public boolean shouldRecord(@NonNull Breadcrumb breadcrumb) { + breadcrumbTasks.clear(); + return true; + } + }); + client.beforeRecordBreadcrumb(new BeforeRecordBreadcrumb() { + @Override + public boolean shouldRecord(@NonNull Breadcrumb breadcrumb) { + return true; + } + }); + client.leaveBreadcrumb("Whoops"); + client.notify(new RuntimeException()); + } + + } From 1bd636c3c115ea773c779021fdb12916dadc2804 Mon Sep 17 00:00:00 2001 From: fractalwrench Date: Fri, 9 Mar 2018 13:48:48 +0000 Subject: [PATCH 3/4] update configuration to use concurrentlinkedqueue for holding callbacks --- .../java/com/bugsnag/android/Configuration.java | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/sdk/src/main/java/com/bugsnag/android/Configuration.java b/sdk/src/main/java/com/bugsnag/android/Configuration.java index 2c9a9780d5..eff55eafce 100644 --- a/sdk/src/main/java/com/bugsnag/android/Configuration.java +++ b/sdk/src/main/java/com/bugsnag/android/Configuration.java @@ -13,6 +13,7 @@ import java.util.Map; import java.util.Observable; import java.util.Observer; +import java.util.concurrent.ConcurrentLinkedQueue; /** * User-specified configuration storage object, contains information @@ -50,9 +51,9 @@ public class Configuration extends Observable implements Observer { @NonNull private MetaData metaData; - private final Collection beforeNotifyTasks = new LinkedHashSet<>(); + private final Collection beforeNotifyTasks = new ConcurrentLinkedQueue<>(); private final Collection beforeRecordBreadcrumbTasks - = new LinkedHashSet<>(); + = new ConcurrentLinkedQueue<>(); private String codeBundleId; private String notifierType; @@ -547,7 +548,9 @@ protected boolean shouldIgnoreClass(String className) { * @param beforeNotify the new before notify task */ protected void beforeNotify(BeforeNotify beforeNotify) { - this.beforeNotifyTasks.add(beforeNotify); + if (!beforeNotifyTasks.contains(beforeNotify)) { + beforeNotifyTasks.add(beforeNotify); + } } /** @@ -556,7 +559,9 @@ protected void beforeNotify(BeforeNotify beforeNotify) { * @param beforeRecordBreadcrumb the new before breadcrumb task */ protected void beforeRecordBreadcrumb(BeforeRecordBreadcrumb beforeRecordBreadcrumb) { - this.beforeRecordBreadcrumbTasks.add(beforeRecordBreadcrumb); + if (!beforeRecordBreadcrumbTasks.contains(beforeRecordBreadcrumb)) { + beforeRecordBreadcrumbTasks.add(beforeRecordBreadcrumb); + } } /** From 8ddfa143adaabf91f6ba69373b469754c5509d1f Mon Sep 17 00:00:00 2001 From: fractalwrench Date: Fri, 9 Mar 2018 13:55:56 +0000 Subject: [PATCH 4/4] update test to use more realistic scenario of adding callbacks --- .../android/ConcurrentCallbackTest.java | 33 +++++++++++-------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/sdk/src/androidTest/java/com/bugsnag/android/ConcurrentCallbackTest.java b/sdk/src/androidTest/java/com/bugsnag/android/ConcurrentCallbackTest.java index 9e46e8f422..7183865b6a 100644 --- a/sdk/src/androidTest/java/com/bugsnag/android/ConcurrentCallbackTest.java +++ b/sdk/src/androidTest/java/com/bugsnag/android/ConcurrentCallbackTest.java @@ -31,16 +31,12 @@ public void testClientNotifyModification() throws Exception { client.beforeNotify(new BeforeNotify() { @Override public boolean run(Error error) { - beforeNotifyTasks.clear(); - return true; - } - }); - client.beforeNotify(new BeforeNotify() { - @Override - public boolean run(Error error) { + beforeNotifyTasks.add(new BeforeNotifySkeleton()); + // modify the Set, when iterating to the next callback this should not crash return true; } }); + client.beforeNotify(new BeforeNotifySkeleton()); client.notify(new RuntimeException()); } @@ -51,19 +47,28 @@ public void testClientBreadcrumbModification() throws Exception { client.beforeRecordBreadcrumb(new BeforeRecordBreadcrumb() { @Override public boolean shouldRecord(@NonNull Breadcrumb breadcrumb) { - breadcrumbTasks.clear(); - return true; - } - }); - client.beforeRecordBreadcrumb(new BeforeRecordBreadcrumb() { - @Override - public boolean shouldRecord(@NonNull Breadcrumb breadcrumb) { + breadcrumbTasks.add(new BeforeRecordBreadcrumbSkeleton()); + // modify the Set, when iterating to the next callback this should not crash return true; } }); + client.beforeRecordBreadcrumb(new BeforeRecordBreadcrumbSkeleton()); client.leaveBreadcrumb("Whoops"); client.notify(new RuntimeException()); } + static class BeforeNotifySkeleton implements BeforeNotify { + @Override + public boolean run(Error error) { + return true; + } + } + + static class BeforeRecordBreadcrumbSkeleton implements BeforeRecordBreadcrumb { + @Override + public boolean shouldRecord(@NonNull Breadcrumb breadcrumb) { + return true; + } + } }