Skip to content

Commit 128906a

Browse files
jaeoptjaeopt
authored
fix(datafileRescheduler): move config file read to a background thread (#401)
- Move config-file read to a background thread when DatafileRescheduler is invoked. This will fix a potential source of ANRs.
1 parent fe92ddc commit 128906a

File tree

2 files changed

+104
-66
lines changed

2 files changed

+104
-66
lines changed

‎datafile-handler/src/androidTest/java/com/optimizely/ab/android/datafile_handler/DatafileReschedulerTest.java

Lines changed: 73 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/****************************************************************************
2-
* Copyright 2016,2021, Optimizely, Inc. and contributors *
2+
* Copyright 2016, 2021-2022, Optimizely, Inc. and contributors *
33
* *
44
* Licensed under the Apache License, Version 2.0 (the "License"); *
55
* you may not use this file except in compliance with the License. *
@@ -21,45 +21,72 @@
2121
import android.content.Intent;
2222
import android.os.Build;
2323

24+
import androidx.test.filters.SdkSuppress;
2425
import androidx.test.platform.app.InstrumentationRegistry;
2526

2627
import com.optimizely.ab.android.shared.Cache;
2728
import com.optimizely.ab.android.shared.DatafileConfig;
2829

30+
import org.junit.After;
2931
import org.junit.Before;
30-
import org.junit.Ignore;
3132
import org.junit.Test;
3233
import org.junit.runner.RunWith;
3334
import org.junit.runners.JUnit4;
3435
import org.mockito.ArgumentCaptor;
3536
import org.slf4j.Logger;
3637

3738
import static junit.framework.Assert.assertEquals;
39+
import static junit.framework.Assert.fail;
3840
import static org.mockito.Matchers.any;
41+
import static org.mockito.Mockito.doNothing;
3942
import static org.mockito.Mockito.mock;
43+
import static org.mockito.Mockito.never;
44+
import static org.mockito.Mockito.spy;
4045
import static org.mockito.Mockito.times;
4146
import static org.mockito.Mockito.verify;
4247
import static org.mockito.Mockito.when;
4348

49+
import java.util.ArrayList;
50+
import java.util.List;
51+
import java.util.concurrent.ExecutorService;
52+
import java.util.concurrent.Executors;
53+
import java.util.concurrent.TimeUnit;
54+
4455
/**
4556
* Tests for {@link DatafileRescheduler}
4657
*/
4758
@RunWith(JUnit4.class)
48-
@Ignore
49-
// Tests pass locally but not on travis
50-
// probably starting too many services
5159
public class DatafileReschedulerTest {
5260

5361
private DatafileRescheduler datafileRescheduler;
5462
private Logger logger;
63+
private Context context;
64+
private Cache cache;
65+
private BackgroundWatchersCache backgroundWatchersCache;
66+
private DatafileRescheduler.Dispatcher dispatcher;
67+
private ArgumentCaptor<DatafileConfig> captor;
5568

5669
@Before
5770
public void setup() {
58-
datafileRescheduler = new DatafileRescheduler();
71+
context = mock(Context.class);
5972
logger = mock(Logger.class);
73+
cache = new Cache(InstrumentationRegistry.getInstrumentation().getTargetContext(), logger);
74+
cache.delete(BackgroundWatchersCache.BACKGROUND_WATCHERS_FILE_NAME);
75+
backgroundWatchersCache = new BackgroundWatchersCache(cache, logger);
76+
captor = ArgumentCaptor.forClass(DatafileConfig.class);
77+
78+
dispatcher = spy(new DatafileRescheduler.Dispatcher(context, backgroundWatchersCache, logger));
79+
doNothing().when(dispatcher).rescheduleService(any());
80+
81+
datafileRescheduler = new DatafileRescheduler();
6082
datafileRescheduler.logger = logger;
6183
}
6284

85+
@After
86+
public void teardown() {
87+
cache.delete(BackgroundWatchersCache.BACKGROUND_WATCHERS_FILE_NAME);
88+
}
89+
6390
@Test
6491
public void receivingNullContext() {
6592
datafileRescheduler.onReceive(null, mock(Intent.class));
@@ -91,64 +118,61 @@ public void receivedActionMyPackageReplaced() {
91118
}
92119

93120
@Test
94-
public void dispatchingOneWithoutEnvironment() {
95-
Context mockContext = mock(Context.class);
96-
Cache cache = new Cache(InstrumentationRegistry.getInstrumentation().getTargetContext(), logger);
97-
BackgroundWatchersCache backgroundWatchersCache = new BackgroundWatchersCache(cache, logger);
121+
@SdkSuppress(maxSdkVersion = Build.VERSION_CODES.N)
122+
public void dispatchingOneWithoutEnvironment() throws InterruptedException {
123+
// projectId: number string
124+
// sdkKey: alphabet string
98125
backgroundWatchersCache.setIsWatching(new DatafileConfig("1", null), true);
99-
Logger logger = mock(Logger.class);
100-
DatafileRescheduler.Dispatcher dispatcher = new DatafileRescheduler.Dispatcher(mockContext, backgroundWatchersCache, logger);
101126
dispatcher.dispatch();
102-
ArgumentCaptor<Intent> captor = ArgumentCaptor.forClass(Intent.class);
103-
verify(mockContext).startService(captor.capture());
104-
assertEquals(new DatafileConfig("1", null).toJSONString(), captor.getValue().getStringExtra(DatafileService.EXTRA_DATAFILE_CONFIG));
105-
verify(logger).info("Rescheduled data file watching for project {}", "1");
106-
cache.delete(BackgroundWatchersCache.BACKGROUND_WATCHERS_FILE_NAME);
127+
TimeUnit.SECONDS.sleep(1);
128+
129+
verify(dispatcher).rescheduleService(captor.capture());
130+
assertEquals(new DatafileConfig("1", null), captor.getValue());
107131
}
108132

109133
@Test
110-
public void dispatchingOneWithEnvironment() {
111-
Context mockContext = mock(Context.class);
112-
Cache cache = new Cache(InstrumentationRegistry.getInstrumentation().getTargetContext(), logger);
113-
BackgroundWatchersCache backgroundWatchersCache = new BackgroundWatchersCache(cache, logger);
114-
backgroundWatchersCache.setIsWatching(new DatafileConfig("1", "2"), true);
115-
Logger logger = mock(Logger.class);
116-
DatafileRescheduler.Dispatcher dispatcher = new DatafileRescheduler.Dispatcher(mockContext, backgroundWatchersCache, logger);
134+
@SdkSuppress(maxSdkVersion = Build.VERSION_CODES.N)
135+
public void dispatchingOneWithEnvironment() throws InterruptedException {
136+
// projectId: number string
137+
// sdkKey: alphabet string
138+
backgroundWatchersCache.setIsWatching(new DatafileConfig(null, "A"), true);
117139
dispatcher.dispatch();
118-
ArgumentCaptor<Intent> captor = ArgumentCaptor.forClass(Intent.class);
119-
verify(mockContext).startService(captor.capture());
120-
assertEquals(new DatafileConfig("1", "2").toJSONString(), captor.getValue().getStringExtra(DatafileService.EXTRA_DATAFILE_CONFIG));
121-
verify(logger).info("Rescheduled data file watching for project {}", "2");
122-
cache.delete(BackgroundWatchersCache.BACKGROUND_WATCHERS_FILE_NAME);
140+
TimeUnit.SECONDS.sleep(1);
141+
142+
verify(dispatcher).rescheduleService(captor.capture());
143+
assertEquals(new DatafileConfig(null, "A"), captor.getValue());
123144
}
124145

125146
@Test
126-
public void dispatchingManyWithoutEnvironment() {
127-
Context mockContext = mock(Context.class);
128-
Cache cache = new Cache(InstrumentationRegistry.getInstrumentation().getTargetContext(), logger);
129-
BackgroundWatchersCache backgroundWatchersCache = new BackgroundWatchersCache(cache, logger);
147+
@SdkSuppress(maxSdkVersion = Build.VERSION_CODES.N)
148+
public void dispatchingManyForLegacy() throws InterruptedException {
130149
backgroundWatchersCache.setIsWatching(new DatafileConfig("1", null), true);
131-
backgroundWatchersCache.setIsWatching(new DatafileConfig("2", null), true);
150+
backgroundWatchersCache.setIsWatching(new DatafileConfig("2", "A"), true);
151+
backgroundWatchersCache.setIsWatching(new DatafileConfig(null, "B"), true);
132152
backgroundWatchersCache.setIsWatching(new DatafileConfig("3", null), true);
133-
Logger logger = mock(Logger.class);
134-
DatafileRescheduler.Dispatcher dispatcher = new DatafileRescheduler.Dispatcher(mockContext, backgroundWatchersCache, logger);
135153
dispatcher.dispatch();
136-
verify(mockContext, times(3)).startService(any(Intent.class));
137-
cache.delete(BackgroundWatchersCache.BACKGROUND_WATCHERS_FILE_NAME);
154+
TimeUnit.SECONDS.sleep(1);
155+
156+
verify(dispatcher, times(4)).rescheduleService(captor.capture());
157+
List array = new ArrayList<String>();
158+
for(DatafileConfig config : captor.getAllValues()) {
159+
array.add(config.toString());
160+
}
161+
assert(array.contains(new DatafileConfig("1", null).toString()));
162+
assert(array.contains(new DatafileConfig(null, "A").toString()));
163+
assert(array.contains(new DatafileConfig(null, "B").toString()));
164+
assert(array.contains(new DatafileConfig("3", null).toString()));
138165
}
139166

140167
@Test
141-
public void dispatchingManyWithEnvironment() {
142-
Context mockContext = mock(Context.class);
143-
Cache cache = new Cache(InstrumentationRegistry.getInstrumentation().getTargetContext(), logger);
144-
BackgroundWatchersCache backgroundWatchersCache = new BackgroundWatchersCache(cache, logger);
145-
backgroundWatchersCache.setIsWatching(new DatafileConfig("1", "1"), true);
146-
backgroundWatchersCache.setIsWatching(new DatafileConfig("2", "1"), true);
147-
backgroundWatchersCache.setIsWatching(new DatafileConfig("3", "1"), true);
148-
Logger logger = mock(Logger.class);
149-
DatafileRescheduler.Dispatcher dispatcher = new DatafileRescheduler.Dispatcher(mockContext, backgroundWatchersCache, logger);
168+
@SdkSuppress(minSdkVersion = Build.VERSION_CODES.O)
169+
public void dispatchingManyForJobScheduler() throws InterruptedException {
170+
backgroundWatchersCache.setIsWatching(new DatafileConfig("1", null), true);
171+
backgroundWatchersCache.setIsWatching(new DatafileConfig(null, "A"), true);
150172
dispatcher.dispatch();
151-
verify(mockContext, times(3)).startService(any(Intent.class));
152-
cache.delete(BackgroundWatchersCache.BACKGROUND_WATCHERS_FILE_NAME);
173+
TimeUnit.SECONDS.sleep(1);
174+
175+
verify(dispatcher, never()).rescheduleService(captor.capture());
153176
}
177+
154178
}

‎datafile-handler/src/main/java/com/optimizely/ab/android/datafile_handler/DatafileRescheduler.java

Lines changed: 31 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/****************************************************************************
2-
* Copyright 2016-2021, Optimizely, Inc. and contributors *
2+
* Copyright 2016-2022, Optimizely, Inc. and contributors *
33
* *
44
* Licensed under the Apache License, Version 2.0 (the "License"); *
55
* you may not use this file except in compliance with the License. *
@@ -22,6 +22,7 @@
2222
import android.os.Build;
2323

2424
import androidx.annotation.NonNull;
25+
import androidx.annotation.VisibleForTesting;
2526

2627
import com.optimizely.ab.android.shared.Cache;
2728
import com.optimizely.ab.android.shared.DatafileConfig;
@@ -66,8 +67,6 @@ public void onReceive(Context context, Intent intent) {
6667
LoggerFactory.getLogger(BackgroundWatchersCache.class));
6768
Dispatcher dispatcher = new Dispatcher(context, backgroundWatchersCache, LoggerFactory.getLogger(Dispatcher.class));
6869
dispatcher.dispatch();
69-
70-
7170
} else {
7271
logger.warn("Received invalid broadcast to data file rescheduler");
7372
}
@@ -78,7 +77,8 @@ public void onReceive(Context context, Intent intent) {
7877
*
7978
* This abstraction mostly makes unit testing easier
8079
*/
81-
static class Dispatcher {
80+
@VisibleForTesting
81+
public static class Dispatcher {
8282

8383
@NonNull private final Context context;
8484
@NonNull private final BackgroundWatchersCache backgroundWatchersCache;
@@ -91,21 +91,35 @@ static class Dispatcher {
9191
}
9292

9393
void dispatch() {
94-
List<DatafileConfig> datafileConfigs = backgroundWatchersCache.getWatchingDatafileConfigs();
95-
96-
for (DatafileConfig datafileConfig : datafileConfigs) {
97-
// for scheduled jobs Android O and above, we use the JobScheduler and persistent periodic jobs
98-
// so, we don't need to do anything.
99-
if (android.os.Build.VERSION.SDK_INT < Build.VERSION_CODES.O) {
100-
WorkerScheduler.scheduleService(context,
101-
DatafileWorker.workerId + datafileConfig.getKey(),
102-
DatafileWorker.class,
103-
DatafileWorker.getData(datafileConfig),
104-
DefaultDatafileHandler.getUpdateInterval(context));
105-
logger.info("Rescheduled data file watching for project {}", datafileConfig);
94+
Thread thread = new Thread() {
95+
@Override
96+
public void run() {
97+
// for scheduled jobs Android O and above, we use the JobScheduler and persistent periodic jobs
98+
// so, we don't need to do anything.
99+
if (android.os.Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
100+
logger.debug("Rescheduling datafile will be done by JobScheduler");
101+
return;
102+
}
103+
104+
// read config file in background thread
105+
List<DatafileConfig> datafileConfigs = backgroundWatchersCache.getWatchingDatafileConfigs();
106+
for (DatafileConfig datafileConfig : datafileConfigs) {
107+
rescheduleService(datafileConfig);
108+
logger.info("Rescheduled datafile watching for project {}", datafileConfig);
109+
}
106110
}
107-
}
111+
};
112+
113+
thread.start();
114+
}
108115

116+
@VisibleForTesting
117+
public void rescheduleService(DatafileConfig datafileConfig) {
118+
WorkerScheduler.scheduleService(context,
119+
DatafileWorker.workerId + datafileConfig.getKey(),
120+
DatafileWorker.class,
121+
DatafileWorker.getData(datafileConfig),
122+
DefaultDatafileHandler.getUpdateInterval(context));
109123
}
110124
}
111125
}

0 commit comments

Comments
 (0)