Skip to content
This repository has been archived by the owner on Apr 8, 2020. It is now read-only.

Commit

Permalink
Merge pull request #167 from firebase/moe_writing_branch_from_d8ce59d…
Browse files Browse the repository at this point in the history
…f571cf56fc0947e1bbc6f345b42a92090

Fixes GooglePlayReceiver synchronization and leaking connections
  • Loading branch information
unEgor committed Oct 7, 2017
2 parents 305223c + 43d49c7 commit 7faa4e1
Show file tree
Hide file tree
Showing 18 changed files with 552 additions and 144 deletions.
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ Play services is unavailable.<br>
Add the following to your `build.gradle`'s dependencies section:

```
compile 'com.firebase:firebase-jobdispatcher:0.8.2'
compile 'com.firebase:firebase-jobdispatcher:0.8.3'
```

### Usage
Expand Down
8 changes: 6 additions & 2 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,18 @@ buildscript {
}

dependencies {
classpath 'com.android.tools.build:gradle:2.2.2'
classpath 'com.android.tools.build:gradle:2.3.1'
classpath 'com.jfrog.bintray.gradle:gradle-bintray-plugin:1.7'
classpath 'com.github.dcendents:android-maven-gradle-plugin:1.4.1'
}
}

allprojects { project ->
repositories {
jcenter()
maven {
url "https://maven.google.com"
}
}

project.apply from: "${rootDir}/common/constants.gradle"
Expand All @@ -30,5 +34,5 @@ task wrapper(type: Wrapper) {
// but requires Gradle 2.13+.
//
// See: https://github.com/jacoco/jacoco/pull/288
gradleVersion = '3.1'
gradleVersion = '3.3'
}
2 changes: 1 addition & 1 deletion common/constants.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
project.ext {
projectName = "firebase-jobdispatcher-android"
group = "com.firebase"
version = "0.8.2"
version = "0.8.3"
buildtools = "25.0.0"
supportLibraryVersion = "25.0.0"
compileSdk = 25
Expand Down
2 changes: 1 addition & 1 deletion gradle/wrapper/gradle-wrapper.properties
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@ distributionBase=GRADLE_USER_HOME
distributionPath=wrapper/dists
zipStoreBase=GRADLE_USER_HOME
zipStorePath=wrapper/dists
distributionUrl=https\://services.gradle.org/distributions/gradle-3.1-all.zip
distributionUrl=https\://services.gradle.org/distributions/gradle-3.3-all.zip
1 change: 1 addition & 0 deletions jobdispatcher/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ android {
dependencies {
// The main library only depends on the Android support lib
compile "com.android.support:support-v4:${project.ext.supportLibraryVersion}"
// compile "com.android.support:support-annotations:${project.ext.supportLibraryVersion}"

def junit = 'junit:junit:4.12'
def robolectric = 'org.robolectric:robolectric:3.3.2'
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,212 @@
// Copyright 2016 Google, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//
////////////////////////////////////////////////////////////////////////////////

package com.firebase.jobdispatcher;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;

import android.content.Context;
import android.support.test.InstrumentationRegistry;
import android.support.test.runner.AndroidJUnit4;
import java.util.HashSet;
import java.util.Set;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;

/** Android test for {@link ExecutionDelegator}. */
@RunWith(AndroidJUnit4.class)
public class ExecutionDelegatorAndroidTest {

private static final int TIMEOUT_SECONDS = 10;

private Context appContext;
private JobInvocation finishedJobInvocation;
private int jobResult;
private ExecutionDelegator executionDelegator;
private final JobInvocation jobInvocation =
new JobInvocation.Builder()
.setTag("tag")
.setService(TestJobService.class.getName())
.setTrigger(Trigger.NOW)
.build();
private volatile CountDownLatch startLatch;
private volatile CountDownLatch stopLatch;

@Before
public void setUp() {
appContext = InstrumentationRegistry.getTargetContext();
TestJobService.reset();
finishedJobInvocation = null;
jobResult = -1;
executionDelegator =
new ExecutionDelegator(
appContext,
/* jobFinishedCallback= */ (jobInvocation, result) -> {
finishedJobInvocation = jobInvocation;
jobResult = result;
});
startLatch = new CountDownLatch(1);
stopLatch = new CountDownLatch(1);

ExecutionDelegator.cleanServiceConnections();

TestJobService.setProxy(
new TestJobService.JobServiceProxy() {
Set<String> runningJobs = new HashSet<>();

@Override
public boolean onStartJob(JobParameters params) {
if (runningJobs.add(params.getTag())) {
startLatch.countDown();
} else {
fail("Job was already started.");
}
return true;
}

@Override
public boolean onStopJob(JobParameters params) {
if (runningJobs.remove(params.getTag())) {
stopLatch.countDown();
} else {
fail("Job was not running.");
}
return false;
}
});
}

@Test
public void execute() throws InterruptedException {
executionDelegator.executeJob(jobInvocation);
assertTrue(
"Latch wasn't counted down as expected",
startLatch.await(TIMEOUT_SECONDS, TimeUnit.SECONDS));
}

@Test
public void executeAndStopWithResult() throws InterruptedException {
executionDelegator.executeJob(jobInvocation);
assertTrue("Job should be started.", startLatch.await(TIMEOUT_SECONDS, TimeUnit.SECONDS));

verifyStopRequestWasProcessed(true);
}

@Test
public void executeAndStopWithoutResult() throws InterruptedException {
executionDelegator.executeJob(jobInvocation);
assertTrue("Job should be started.", startLatch.await(TIMEOUT_SECONDS, TimeUnit.SECONDS));

verifyStopRequestWasProcessed(false);
}

@Test
public void execute_unbind_jobStopped() throws InterruptedException {
executionDelegator.executeJob(jobInvocation);
assertTrue("Job should be started.", startLatch.await(TIMEOUT_SECONDS, TimeUnit.SECONDS));

synchronized (ExecutionDelegator.serviceConnections) {
ExecutionDelegator.serviceConnections.get(jobInvocation).unbind();
}

assertTrue("Job should be stopped.", stopLatch.await(TIMEOUT_SECONDS, TimeUnit.SECONDS));
}

@Test
public void execute_twice_stopAndRestart() throws InterruptedException {
executionDelegator.executeJob(jobInvocation);
assertTrue("Job should be started.", startLatch.await(TIMEOUT_SECONDS, TimeUnit.SECONDS));

startLatch = new CountDownLatch(1);

executionDelegator.executeJob(jobInvocation);

assertTrue("First job should be stopped.", stopLatch.await(TIMEOUT_SECONDS, TimeUnit.SECONDS));
assertTrue("Job should be started again.", startLatch.await(TIMEOUT_SECONDS, TimeUnit.SECONDS));
}

@Test
public void execute_afterStopWithResult_jobServiceStartedAgain() throws InterruptedException {
executionDelegator.executeJob(jobInvocation);
assertTrue("Job should be started.", startLatch.await(TIMEOUT_SECONDS, TimeUnit.SECONDS));

verifyStopRequestWasProcessed(true);

ExecutionDelegator.stopJob(jobInvocation, true);
ExecutionDelegator.stopJob(jobInvocation, true); // should not throw when called again

startLatch = new CountDownLatch(1);

executionDelegator.executeJob(jobInvocation);
assertTrue("Job should be started again.", startLatch.await(TIMEOUT_SECONDS, TimeUnit.SECONDS));
}

@Test
public void executeTwoJobs_stopFirst_secondStays() throws InterruptedException {
JobInvocation secondJobInvocation =
new JobInvocation.Builder()
.setTag("secondJob")
.setService(TestJobService.class.getName())
.setTrigger(Trigger.NOW)
.build();

executionDelegator.executeJob(jobInvocation);
assertTrue("First job should be started.", startLatch.await(TIMEOUT_SECONDS, TimeUnit.SECONDS));

startLatch = new CountDownLatch(1);
executionDelegator.executeJob(secondJobInvocation);
assertTrue(
"Second job should be started.", startLatch.await(TIMEOUT_SECONDS, TimeUnit.SECONDS));

ExecutionDelegator.stopJob(jobInvocation, false);

assertTrue("First job should be stopped.", stopLatch.await(TIMEOUT_SECONDS, TimeUnit.SECONDS));

synchronized (ExecutionDelegator.serviceConnections) {
assertFalse(ExecutionDelegator.serviceConnections.get(secondJobInvocation).wasUnbound());
}

startLatch = new CountDownLatch(1);
executionDelegator.executeJob(jobInvocation);
assertTrue(
"First job should be started again because it was stopped.",
startLatch.await(TIMEOUT_SECONDS, TimeUnit.SECONDS));
}

private void verifyStopRequestWasProcessed(boolean withResult) throws InterruptedException {
ExecutionDelegator.stopJob(jobInvocation, withResult);

CountDownLatch latch = new CountDownLatch(1);
executionDelegator.responseHandler.post(() -> latch.countDown());
assertTrue(
"All messages need to be processed.", latch.await(TIMEOUT_SECONDS, TimeUnit.SECONDS));

if (withResult) {
assertEquals(jobInvocation, finishedJobInvocation);
assertEquals(JobService.RESULT_SUCCESS, jobResult);
} else {
assertNull(finishedJobInvocation);
assertEquals(-1, jobResult);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import android.os.Handler;
import android.os.Looper;
import android.os.Message;
// import android.support.annotation.GuardedBy;
import android.support.annotation.NonNull;
import android.support.annotation.VisibleForTesting;
import android.support.v4.util.SimpleArrayMap;
Expand All @@ -45,11 +46,21 @@ interface JobFinishedCallback {

/** A mapping of {@link JobInvocation} to (local) binder connections. Synchronized by itself. */
@VisibleForTesting
// @GuardedBy("serviceConnections")
static final SimpleArrayMap<JobInvocation, JobServiceConnection> serviceConnections =
new SimpleArrayMap<>();

private final ResponseHandler responseHandler =
@VisibleForTesting
static void cleanServiceConnections() {
synchronized (serviceConnections) {
serviceConnections.clear();
}
}

@VisibleForTesting
final ResponseHandler responseHandler =
new ResponseHandler(Looper.getMainLooper(), new WeakReference<>(this));

private final Context context;
private final JobFinishedCallback jobFinishedCallback;

Expand All @@ -61,24 +72,31 @@ interface JobFinishedCallback {
/**
* Executes the provided {@code jobInvocation} by kicking off the creation of a new Binder
* connection to the Service.
*
* @return true if the service was bound successfully.
*/
boolean executeJob(JobInvocation jobInvocation) {
void executeJob(JobInvocation jobInvocation) {
if (jobInvocation == null) {
return false;
return;
}

JobServiceConnection conn =
new JobServiceConnection(
jobInvocation, responseHandler.obtainMessage(JOB_FINISHED), context);

synchronized (serviceConnections) {
JobServiceConnection oldConnection = serviceConnections.put(jobInvocation, conn);
JobServiceConnection oldConnection = serviceConnections.get(jobInvocation);
if (oldConnection != null) {
Log.e(TAG, "Received execution request for already running job");
if (!oldConnection.isConnected() && !oldConnection.wasUnbound()) {
// Fresh connection. Not yet connected or not able to connect.
// TODO(user) Handle invalid service when the connection can't be established.
return;
}
oldConnection.onStop(false /* Do not send result because it is new execution request. */);
}
JobServiceConnection conn =
new JobServiceConnection(
jobInvocation, responseHandler.obtainMessage(JOB_FINISHED), context);

serviceConnections.put(jobInvocation, conn);
if (!context.bindService(createBindIntent(jobInvocation), conn, BIND_AUTO_CREATE)) {
Log.e(TAG, "Unable to bind to " + jobInvocation.getService());
conn.unbind();
}
return context.bindService(createBindIntent(jobInvocation), conn, BIND_AUTO_CREATE);
}
}

Expand Down Expand Up @@ -109,7 +127,8 @@ private void onJobFinishedMessage(JobInvocation jobInvocation, int result) {
jobFinishedCallback.onJobFinished(jobInvocation, result);
}

private static class ResponseHandler extends Handler {
@VisibleForTesting
static class ResponseHandler extends Handler {

/**
* We hold a WeakReference to the ExecutionDelegator because it holds a reference to a Service
Expand All @@ -130,8 +149,7 @@ public void handleMessage(Message msg) {
if (msg.obj instanceof JobInvocation) {
ExecutionDelegator delegator = this.executionDelegatorReference.get();
if (delegator == null) {
Log.wtf(
TAG, "handleMessage: service was unexpectedly GC'd" + ", can't send job result");
Log.wtf(TAG, "handleMessage: service was unexpectedly GC'd, can't send job result");
return;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import android.os.IBinder;
import android.os.Parcel;
import android.os.Parcelable;
// import android.support.annotation.GuardedBy;
import android.support.annotation.Nullable;
import android.util.Log;
import android.util.Pair;
Expand All @@ -47,7 +48,7 @@
/** A magic number that indicates the following value is a Parcelable. */
private static final int VAL_PARCELABLE = 4;

// GuardedBy("GooglePlayCallbackExtractor.class")
// @GuardedBy("GooglePlayCallbackExtractor.class")
private static Boolean shouldReadKeysAsStringsCached = null;

public Pair<JobCallback, Bundle> extractCallback(@Nullable Bundle data) {
Expand Down

0 comments on commit 7faa4e1

Please sign in to comment.