From 4e021d8484356ddaaad8ae0dbeb4001131a126d1 Mon Sep 17 00:00:00 2001 From: Mikhail Lopatkin Date: Mon, 25 Dec 2023 12:04:44 +0100 Subject: [PATCH] Add method to discard current ADB services The connection can then be re-established Issue: #197 --- base/build.gradle.kts | 1 + .../ExtendedCompletableFutureAssert.java | 82 +++++++++++++++++++ .../ui/device/AdbServicesBridge.java | 25 ++++-- .../ui/device/AdbServicesBridgeTest.java | 72 ++++++++++++++++ 4 files changed, 175 insertions(+), 5 deletions(-) create mode 100644 base/src/testFixtures/java/name/mlopatkin/andlogview/base/concurrent/ExtendedCompletableFutureAssert.java diff --git a/base/build.gradle.kts b/base/build.gradle.kts index 9c8a0ca2..2a1be6c5 100644 --- a/base/build.gradle.kts +++ b/base/build.gradle.kts @@ -30,6 +30,7 @@ dependencies { implementation(libs.guava) implementation(libs.log4j) + testFixturesApi(libs.test.assertj) testFixturesApi(libs.test.hamcrest.hamcrest) testFixturesImplementation(libs.guava) testFixturesImplementation(libs.test.mockito.core) diff --git a/base/src/testFixtures/java/name/mlopatkin/andlogview/base/concurrent/ExtendedCompletableFutureAssert.java b/base/src/testFixtures/java/name/mlopatkin/andlogview/base/concurrent/ExtendedCompletableFutureAssert.java new file mode 100644 index 00000000..61678fce --- /dev/null +++ b/base/src/testFixtures/java/name/mlopatkin/andlogview/base/concurrent/ExtendedCompletableFutureAssert.java @@ -0,0 +1,82 @@ +/* + * Copyright 2023 the Andlogview authors + * + * 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 name.mlopatkin.andlogview.base.concurrent; + +import name.mlopatkin.andlogview.base.MyThrowables; + +import com.google.common.base.Preconditions; + +import org.assertj.core.api.AbstractCompletableFutureAssert; +import org.assertj.core.api.CompletableFutureAssert; + +import java.util.concurrent.CancellationException; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CompletionException; +import java.util.concurrent.ExecutionException; + +/** + * Extended version of AssertJ's {@link CompletableFutureAssert}. + * + * @param the future's result type + */ +public class ExtendedCompletableFutureAssert + extends AbstractCompletableFutureAssert, RESULT> { + public ExtendedCompletableFutureAssert(CompletableFuture actual) { + super(actual, ExtendedCompletableFutureAssert.class); + } + + /** + * Entry point to the completable future assertions (cannot use assertThat to avoid clashing with the existing + * method). + * + * @param future the future to assert + * @param the future's result type + * @return the assertion + */ + public static ExtendedCompletableFutureAssert assertThatCompletableFuture( + CompletableFuture future) { + return new ExtendedCompletableFutureAssert<>(future); + } + + /** + * Verifies that the {@link CompletableFuture} is cancelled or failed because one of its upstreams has been + * cancelled. + * + * @return this assertion object + */ + public ExtendedCompletableFutureAssert isCancelledUpstream() { + super.isCompletedExceptionally(); + var failureReason = getFailureReason(); + if (!(failureReason instanceof CancellationException)) { + failWithMessage("Expected failure reason to be CancellationException but got %s", failureReason); + } + return this; + } + + + private Throwable getFailureReason() { + Preconditions.checkState(actual.isCompletedExceptionally(), "The future is not yet completed"); + try { + actual.get(); + throw new AssertionError("Expecting failure to be present"); + } catch (ExecutionException | CancellationException | CompletionException e) { + return MyThrowables.unwrapUninteresting(e); + } catch (InterruptedException e) { + throw new AssertionError("Completed future cannot be interrupted"); + } + } +} diff --git a/src/name/mlopatkin/andlogview/ui/device/AdbServicesBridge.java b/src/name/mlopatkin/andlogview/ui/device/AdbServicesBridge.java index aca5cf00..9dfd9417 100644 --- a/src/name/mlopatkin/andlogview/ui/device/AdbServicesBridge.java +++ b/src/name/mlopatkin/andlogview/ui/device/AdbServicesBridge.java @@ -69,8 +69,6 @@ public class AdbServicesBridge implements AdbServicesStatus { private final GlobalAdbDeviceList adbDeviceList; private final Subject statusObservers = new Subject<>(); - // TODO(mlopatkin) My attempt to hide connection replacement logic in the AdbServer failed. What if the connection - // update fails? The server would be unusable and we have to discard the whole component. private @Nullable CompletableFuture adbSubcomponent; // This one is three-state. The `null` here // means that nobody attempted to initialize ADB yet. Non-null holds the current subcomponent if it was created // successfully or the initialization error if something failed. @@ -91,7 +89,7 @@ public class AdbServicesBridge implements AdbServicesStatus { /** * Tries to create AdbServices, potentially initializing ADB connection if is it is not ready yet. - * This may fail and show an error dialog. + * This may fail, or may be cancelled. * * @return a completable future that will provide {@link AdbServices} when ready */ @@ -122,7 +120,7 @@ private CompletableFuture initAdbAsync() { // This is a separate chain, not related to the consumers of getAdbServicesAsync. Therefore, it has a separate // exception sink to handle runtime errors in the handler. result.handleAsync( - consumingHandler((r, th) -> onAdbInitFinished(th, stopwatch)), + consumingHandler((r, th) -> onAdbInitFinished(result, th, stopwatch)), uiExecutor) .exceptionally(MyFutures::uncaughtException); return result; @@ -133,7 +131,12 @@ private AdbServices buildServices(AdbServer adbServer) { return adbSubcomponentFactory.build(adbDeviceList); } - private void onAdbInitFinished(@Nullable Throwable maybeFailure, Stopwatch timeTracing) { + private void onAdbInitFinished(CompletableFuture origin, @Nullable Throwable maybeFailure, + Stopwatch timeTracing) { + if (adbSubcomponent != origin) { + // Our initialization was cancelled by this point. We shouldn't propagate notifications further. + return; + } logger.info("Initialized adb server in " + timeTracing.elapsed(TimeUnit.MILLISECONDS) + "ms"); if (maybeFailure != null) { logger.error("Failed to initialize ADB", maybeFailure); @@ -181,4 +184,16 @@ private static String getAdbFailureString(Throwable th) { .map(Throwable::getMessage) .orElse(MoreObjects.firstNonNull(th.getMessage(), "unknown failure")); } + + /** + * Discards the running adb services subcomponent if any. Any ongoing ADB initialization is cancelled. + */ + public void stopAdb() { + var currentAdb = adbSubcomponent; + if (currentAdb != null) { + adbSubcomponent = null; + notifyStatusChange(getStatus()); + currentAdb.cancel(false); + } + } } diff --git a/test/name/mlopatkin/andlogview/ui/device/AdbServicesBridgeTest.java b/test/name/mlopatkin/andlogview/ui/device/AdbServicesBridgeTest.java index 0255d838..d2bbff78 100644 --- a/test/name/mlopatkin/andlogview/ui/device/AdbServicesBridgeTest.java +++ b/test/name/mlopatkin/andlogview/ui/device/AdbServicesBridgeTest.java @@ -16,12 +16,16 @@ package name.mlopatkin.andlogview.ui.device; +import static name.mlopatkin.andlogview.base.concurrent.ExtendedCompletableFutureAssert.assertThatCompletableFuture; + import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.isA; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; import name.mlopatkin.andlogview.base.concurrent.TestExecutor; @@ -180,6 +184,74 @@ void asyncHandlersSeeProperStatus() { assertThat(bridgeStatus.get()).isEqualTo(StatusValue.initialized()); } + @Test + void serviceFutureIsCancelledIfAdbStopped() { + var testUiExecutor = new TestExecutor(); + var testAdbExecutor = new TestExecutor(); + var bridge = createBridge(testUiExecutor, testAdbExecutor); + + var serviceFuture = bridge.getAdbServicesAsync(); + bridge.stopAdb(); + + assertThatCompletableFuture(serviceFuture).isCancelledUpstream(); + } + + @Test + void adbIsNotInitializedAfterStopped() { + var testUiExecutor = new TestExecutor(); + var testAdbExecutor = new TestExecutor(); + var bridge = createBridge(testUiExecutor, testAdbExecutor); + var observer = mock(AdbServicesStatus.Observer.class); + + var firstInit = bridge.getAdbServicesAsync(); + bridge.stopAdb(); + bridge.asObservable().addObserver(observer); + drainExecutors(testAdbExecutor, testUiExecutor); + + assertThat(bridge.getStatus()).isInstanceOf(AdbServicesStatus.NotInitialized.class); + + assertThat(firstInit).isCompletedExceptionally(); + verifyNoInteractions(observer); + } + + @Test + void completingPreviousAdbInitializationAfterStopDoesNotChangeStatus() { + var testUiExecutor = new TestExecutor(); + var testAdbExecutor = new TestExecutor(); + var bridge = createBridge(testUiExecutor, testAdbExecutor); + var observer = mock(AdbServicesStatus.Observer.class); + + var firstInit = bridge.getAdbServicesAsync(); + testAdbExecutor.flush(); + bridge.stopAdb(); + bridge.asObservable().addObserver(observer); + drainExecutors(testAdbExecutor, testUiExecutor); + + assertThat(firstInit).isCompletedExceptionally(); + assertThat(bridge.getStatus()).isInstanceOf(AdbServicesStatus.NotInitialized.class); + verifyNoInteractions(observer); + } + + @Test + void completingNewAdbInitializationAfterStopSendsNoSpuriousNotifications() { + var testUiExecutor = new TestExecutor(); + var testAdbExecutor = new TestExecutor(); + var bridge = createBridge(testUiExecutor, testAdbExecutor); + var observer = mock(AdbServicesStatus.Observer.class); + + var firstInit = bridge.getAdbServicesAsync(); + bridge.stopAdb(); + var secondInit = bridge.getAdbServicesAsync(); + bridge.asObservable().addObserver(observer); + drainExecutors(testAdbExecutor, testUiExecutor); + + assertThat(firstInit).isCompletedExceptionally(); + assertThat(secondInit).isCompleted().isNotCancelled(); + + verify(observer).onAdbServicesStatusChanged(isA(AdbServicesStatus.Initialized.class)); + verifyNoMoreInteractions(observer); + } + private AdbServicesBridge createBridge() { return createBridge(MoreExecutors.directExecutor(), MoreExecutors.directExecutor()); }