Skip to content

Commit bae84c7

Browse files
vkryachkovkryachko
authored
Simplified synchronization guard interface. (#376)
* Simplified synchronization guard interface. Lock timeout is no longer part of the interface and instead is an implementation detail of SqliteEventStore. * Shutdown event store after tests. This avoids DB locks when multiple instances of event store exist. * Fix
1 parent 654572a commit bae84c7

File tree

17 files changed

+151
-109
lines changed

17 files changed

+151
-109
lines changed

‎transport/transport-runtime/src/androidTest/aidl/com/google/android/datatransport/runtime/ITestRemoteLockRpc.aidl

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ package com.google.android.datatransport.runtime;
44
interface ITestRemoteLockRpc {
55

66
/** Try to acuire lock or timeout. */
7-
boolean tryAcquireLock(long lockTimeoutMs);
7+
boolean tryAcquireLock();
88

99
/** Release held lock. If lock is not held - undefined behavior. */
1010
void releaseLock();

‎transport/transport-runtime/src/androidTest/java/com/google/android/datatransport/runtime/SynchronizationComponent.java

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,19 +15,20 @@
1515
package com.google.android.datatransport.runtime;
1616

1717
import android.content.Context;
18-
import com.google.android.datatransport.runtime.scheduling.persistence.EventStoreModule;
18+
import com.google.android.datatransport.runtime.scheduling.persistence.SQLiteEventStore;
19+
import com.google.android.datatransport.runtime.scheduling.persistence.TestEventStoreModule;
1920
import com.google.android.datatransport.runtime.synchronization.SynchronizationGuard;
2021
import com.google.android.datatransport.runtime.time.TimeModule;
2122
import dagger.BindsInstance;
2223
import dagger.Component;
2324
import javax.inject.Singleton;
2425

25-
@Component(modules = {EventStoreModule.class, TimeModule.class})
26+
@Component(modules = {TestEventStoreModule.class, TimeModule.class})
2627
@Singleton
2728
public abstract class SynchronizationComponent {
28-
private static SynchronizationGuard INSTANCE;
29+
private static SQLiteEventStore INSTANCE;
2930

30-
abstract SynchronizationGuard getGuard();
31+
abstract SQLiteEventStore getGuard();
3132

3233
public static SynchronizationGuard getGuard(Context applicationContext) {
3334
synchronized (SynchronizationComponent.class) {
@@ -38,6 +39,15 @@ public static SynchronizationGuard getGuard(Context applicationContext) {
3839
}
3940
}
4041

42+
public static void shutdown() {
43+
synchronized (SynchronizationComponent.class) {
44+
if (INSTANCE == null) {
45+
return;
46+
}
47+
INSTANCE.close();
48+
}
49+
}
50+
4151
@Component.Factory
4252
protected interface Factory {
4353
SynchronizationComponent create(@BindsInstance Context applicationContext);

‎transport/transport-runtime/src/androidTest/java/com/google/android/datatransport/runtime/scheduling/MultiProcessSynchronizationGuardTest.java

Lines changed: 9 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -19,15 +19,13 @@
1919

2020
import android.app.Service;
2121
import android.content.Intent;
22-
import android.os.IBinder;
2322
import android.os.RemoteException;
24-
import android.os.SystemClock;
2523
import android.support.test.InstrumentationRegistry;
2624
import android.support.test.rule.ServiceTestRule;
2725
import android.support.test.runner.AndroidJUnit4;
2826
import com.google.android.datatransport.runtime.ITestRemoteLockRpc;
29-
import com.google.android.datatransport.runtime.scheduling.persistence.SQLiteEventStore;
3027
import java.util.concurrent.TimeoutException;
28+
import org.junit.After;
3129
import org.junit.Before;
3230
import org.junit.Test;
3331
import org.junit.runner.RunWith;
@@ -48,6 +46,11 @@ public void setUp() throws TimeoutException, RemoteException {
4846
assertThat(process1.getPid()).isNotEqualTo(process2.getPid());
4947
}
5048

49+
@After
50+
public void after() {
51+
rule.unbindService();
52+
}
53+
5154
@Test
5255
public void locking_whenDbConnectionsNotOpenAndLocalProcessLockedFirst_shouldWorkAsExpected()
5356
throws RemoteException {
@@ -92,7 +95,7 @@ private static void doTest_whenConnectionsAreOpen(
9295

9396
private static void lockAndRunOrFail(ITestRemoteLockRpc rpc, ThrowingRunnable runnable)
9497
throws RemoteException {
95-
assertThat(rpc.tryAcquireLock(0)).isTrue();
98+
assertThat(rpc.tryAcquireLock()).isTrue();
9699
try {
97100
runnable.run();
98101
} finally {
@@ -101,7 +104,7 @@ private static void lockAndRunOrFail(ITestRemoteLockRpc rpc, ThrowingRunnable ru
101104
}
102105

103106
private static void assertCanLock(ITestRemoteLockRpc rpc, boolean can) throws RemoteException {
104-
boolean locked = rpc.tryAcquireLock(0);
107+
boolean locked = rpc.tryAcquireLock();
105108

106109
try {
107110
if (locked) {
@@ -123,38 +126,6 @@ private ITestRemoteLockRpc bindTestingRpc(Class<? extends Service> serviceClass)
123126
ITestRemoteLockRpc rpc =
124127
ITestRemoteLockRpc.Stub.asInterface(
125128
rule.bindService(new Intent(InstrumentationRegistry.getTargetContext(), serviceClass)));
126-
return new WaitingRpc(rpc);
127-
}
128-
129-
static class WaitingRpc implements ITestRemoteLockRpc {
130-
private final ITestRemoteLockRpc delegate;
131-
132-
WaitingRpc(ITestRemoteLockRpc delegate) {
133-
this.delegate = delegate;
134-
}
135-
136-
@Override
137-
public boolean tryAcquireLock(long timeout) throws RemoteException {
138-
boolean result = delegate.tryAcquireLock(timeout);
139-
if (!result) {
140-
SystemClock.sleep(SQLiteEventStore.LOCK_RETRY_BACK_OFF_MILLIS * 2);
141-
}
142-
return result;
143-
}
144-
145-
@Override
146-
public void releaseLock() throws RemoteException {
147-
delegate.releaseLock();
148-
}
149-
150-
@Override
151-
public long getPid() throws RemoteException {
152-
return delegate.getPid();
153-
}
154-
155-
@Override
156-
public IBinder asBinder() {
157-
return delegate.asBinder();
158-
}
129+
return rpc;
159130
}
160131
}

‎transport/transport-runtime/src/androidTest/java/com/google/android/datatransport/runtime/scheduling/RemoteLockRpc.java

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,13 +35,12 @@ class RemoteLockRpc extends ITestRemoteLockRpc.Stub {
3535
}
3636

3737
@Override
38-
public boolean tryAcquireLock(long lockTimeoutMs) {
38+
public boolean tryAcquireLock() {
3939
Locker<Boolean> sectionEnteredLocker = new Locker<>();
4040
executor.execute(
4141
() -> {
4242
try {
4343
guard.runCriticalSection(
44-
lockTimeoutMs,
4544
() -> {
4645
sectionEnteredLocker.setResult(true);
4746
acquireReleaseLocker.await();

‎transport/transport-runtime/src/androidTest/java/com/google/android/datatransport/runtime/scheduling/TestService.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,12 @@ public IBinder onBind(Intent intent) {
3131
DaggerSynchronizationComponent.getGuard(getApplicationContext()));
3232
}
3333

34+
@Override
35+
public boolean onUnbind(Intent intent) {
36+
DaggerSynchronizationComponent.shutdown();
37+
return false;
38+
}
39+
3440
/** Service instance that runs in the main process of the Android application. */
3541
public static class Local extends TestService {}
3642

‎transport/transport-runtime/src/androidTest/java/com/google/android/datatransport/runtime/scheduling/persistence/TestEventStoreModule.java

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
// Copyright 2019 Google LLC
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package com.google.android.datatransport.runtime.scheduling.persistence;
16+
17+
import com.google.android.datatransport.runtime.synchronization.SynchronizationGuard;
18+
import dagger.Binds;
19+
import dagger.Module;
20+
import dagger.Provides;
21+
22+
@Module
23+
public abstract class TestEventStoreModule {
24+
private static final long MAX_DB_STORAGE_SIZE_IN_BYTES = 10 * 1024 * 1024;
25+
private static final int LOAD_BATCH_SIZE = 200;
26+
private static final int LOCK_TIME_OUT_MS = 0;
27+
28+
@Provides
29+
static EventStoreConfig storeConfig() {
30+
return EventStoreConfig.builder()
31+
.setMaxStorageSizeInBytes(MAX_DB_STORAGE_SIZE_IN_BYTES)
32+
.setLoadBatchSize(LOAD_BATCH_SIZE)
33+
.setCriticalSectionEnterTimeoutMs(LOCK_TIME_OUT_MS)
34+
.build();
35+
}
36+
37+
@Binds
38+
abstract EventStore eventStore(SQLiteEventStore store);
39+
40+
@Binds
41+
abstract SynchronizationGuard synchronizationGuard(SQLiteEventStore store);
42+
}

‎transport/transport-runtime/src/main/java/com/google/android/datatransport/runtime/TransportRuntimeComponent.java

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,13 @@
1717
import android.content.Context;
1818
import com.google.android.datatransport.runtime.backends.BackendRegistryModule;
1919
import com.google.android.datatransport.runtime.scheduling.SchedulingModule;
20+
import com.google.android.datatransport.runtime.scheduling.persistence.EventStore;
2021
import com.google.android.datatransport.runtime.scheduling.persistence.EventStoreModule;
2122
import com.google.android.datatransport.runtime.time.TimeModule;
2223
import dagger.BindsInstance;
2324
import dagger.Component;
25+
import java.io.Closeable;
26+
import java.io.IOException;
2427
import javax.inject.Singleton;
2528

2629
@Component(
@@ -32,8 +35,15 @@
3235
TimeModule.class,
3336
})
3437
@Singleton
35-
interface TransportRuntimeComponent {
36-
TransportRuntime getTransportRuntime();
38+
abstract class TransportRuntimeComponent implements Closeable {
39+
abstract TransportRuntime getTransportRuntime();
40+
41+
abstract EventStore getEventStore();
42+
43+
@Override
44+
public void close() throws IOException {
45+
getEventStore().close();
46+
}
3747

3848
@Component.Builder
3949
interface Builder {

‎transport/transport-runtime/src/main/java/com/google/android/datatransport/runtime/scheduling/DefaultScheduler.java

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@ public class DefaultScheduler implements Scheduler {
3838
private final BackendRegistry backendRegistry;
3939
private final EventStore eventStore;
4040
private final SynchronizationGuard guard;
41-
private final int LOCK_TIME_OUT = 10000; // 10 seconds lock timeout
4241

4342
@Inject
4443
DefaultScheduler(
@@ -74,7 +73,6 @@ public void schedule(TransportContext transportContext, EventInternal event) {
7473
}
7574
EventInternal decoratedEvent = transportBackend.decorate(event);
7675
guard.runCriticalSection(
77-
LOCK_TIME_OUT,
7876
() -> {
7977
eventStore.persist(transportContext, decoratedEvent);
8078
workScheduler.schedule(transportContext, 0);

‎transport/transport-runtime/src/main/java/com/google/android/datatransport/runtime/scheduling/jobscheduling/Uploader.java

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,6 @@
3636
/** Handles upload of all the events corresponding to a backend. */
3737
public class Uploader {
3838

39-
private final int LOCK_TIMEOUT = 10000;
4039
private final Context context;
4140
private final BackendRegistry backendRegistry;
4241
private final EventStore eventStore;
@@ -78,7 +77,6 @@ void upload(String backendName, int attemptNumber, Runnable callback) {
7877
TransportContext.builder().setBackendName(backendName).build();
7978
if (!isNetworkAvailable()) {
8079
guard.runCriticalSection(
81-
LOCK_TIMEOUT,
8280
() -> {
8381
workScheduler.schedule(transportContext, attemptNumber + 1);
8482
return null;
@@ -96,7 +94,7 @@ void logAndUpdateState(TransportContext transportContext, int attemptNumber) {
9694
TransportBackend backend = backendRegistry.get(transportContext.getBackendName());
9795
List<EventInternal> eventInternals = new ArrayList<>();
9896
Iterable<PersistedEvent> persistedEvents =
99-
guard.runCriticalSection(LOCK_TIMEOUT, () -> eventStore.loadBatch(transportContext));
97+
guard.runCriticalSection(() -> eventStore.loadBatch(transportContext));
10098

10199
// Donot make a call to the backend if the list is empty.
102100
if (!persistedEvents.iterator().hasNext()) {
@@ -108,7 +106,6 @@ void logAndUpdateState(TransportContext transportContext, int attemptNumber) {
108106
}
109107
BackendResponse response = backend.send(BackendRequest.create(eventInternals));
110108
guard.runCriticalSection(
111-
LOCK_TIMEOUT,
112109
() -> {
113110
if (response.getStatus() == BackendResponse.Status.TRANSIENT_ERROR) {
114111
eventStore.recordFailure(persistedEvents);

‎transport/transport-runtime/src/main/java/com/google/android/datatransport/runtime/scheduling/persistence/EventStore.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,14 +18,15 @@
1818
import android.support.annotation.WorkerThread;
1919
import com.google.android.datatransport.runtime.EventInternal;
2020
import com.google.android.datatransport.runtime.TransportContext;
21+
import java.io.Closeable;
2122

2223
/**
2324
* Persistence layer.
2425
*
2526
* <p>Responsible for storing events and backend-specific metadata.
2627
*/
2728
@WorkerThread
28-
public interface EventStore {
29+
public interface EventStore extends Closeable {
2930

3031
/** Persist a new event. */
3132
@Nullable

‎transport/transport-runtime/src/main/java/com/google/android/datatransport/runtime/scheduling/persistence/EventStoreConfig.java

Lines changed: 31 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -18,23 +18,42 @@
1818

1919
@AutoValue
2020
abstract class EventStoreConfig {
21+
private static final long MAX_DB_STORAGE_SIZE_IN_BYTES = 10 * 1024 * 1024;
22+
private static final int LOAD_BATCH_SIZE = 200;
23+
private static final int LOCK_TIME_OUT_MS = 10000;
24+
25+
static final EventStoreConfig DEFAULT =
26+
EventStoreConfig.builder()
27+
.setMaxStorageSizeInBytes(MAX_DB_STORAGE_SIZE_IN_BYTES)
28+
.setLoadBatchSize(LOAD_BATCH_SIZE)
29+
.setCriticalSectionEnterTimeoutMs(LOCK_TIME_OUT_MS)
30+
.build();
31+
2132
abstract long getMaxStorageSizeInBytes();
2233

2334
abstract int getLoadBatchSize();
2435

25-
static EventStoreConfig create(long maxStorageSizeInBytes, int loadBatchSize) {
26-
if (maxStorageSizeInBytes < 0) {
27-
throw new IllegalArgumentException(
28-
"Cannot set max storage size to a negative number of bytes");
29-
}
30-
if (loadBatchSize < 0) {
31-
throw new IllegalArgumentException(
32-
"Cannot set load batch size to a negative number of bytes");
33-
}
34-
return new AutoValue_EventStoreConfig(maxStorageSizeInBytes, loadBatchSize);
36+
abstract int getCriticalSectionEnterTimeoutMs();
37+
38+
static EventStoreConfig.Builder builder() {
39+
return new AutoValue_EventStoreConfig.Builder();
40+
}
41+
42+
Builder toBuilder() {
43+
return builder()
44+
.setMaxStorageSizeInBytes(getMaxStorageSizeInBytes())
45+
.setLoadBatchSize(getLoadBatchSize())
46+
.setCriticalSectionEnterTimeoutMs(getCriticalSectionEnterTimeoutMs());
3547
}
3648

37-
EventStoreConfig withMaxStorageSizeInBytes(long newVaue) {
38-
return create(newVaue, getLoadBatchSize());
49+
@AutoValue.Builder
50+
abstract static class Builder {
51+
abstract Builder setMaxStorageSizeInBytes(long value);
52+
53+
abstract Builder setLoadBatchSize(int value);
54+
55+
abstract Builder setCriticalSectionEnterTimeoutMs(int value);
56+
57+
abstract EventStoreConfig build();
3958
}
4059
}

‎transport/transport-runtime/src/main/java/com/google/android/datatransport/runtime/scheduling/persistence/EventStoreModule.java

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,12 +21,9 @@
2121

2222
@Module
2323
public abstract class EventStoreModule {
24-
private static final long MAX_DB_STORAGE_SIZE_IN_BYTES = 10 * 1024 * 1024;
25-
private static final int LOAD_BATCH_SIZE = 200;
26-
2724
@Provides
2825
static EventStoreConfig storeConfig() {
29-
return EventStoreConfig.create(MAX_DB_STORAGE_SIZE_IN_BYTES, LOAD_BATCH_SIZE);
26+
return EventStoreConfig.DEFAULT;
3027
}
3128

3229
@Binds

0 commit comments

Comments
 (0)