فهرست منبع

Further Refactoring:
* simplyify check executor by using Fx TaskAPI
* reduce number of check states

Armin Schrenk 4 سال پیش
والد
کامیت
19c61ffea8

+ 15 - 3
src/main/java/org/cryptomator/ui/health/Check.java

@@ -1,5 +1,6 @@
 package org.cryptomator.ui.health;
 
+import org.cryptomator.cryptofs.health.api.DiagnosticResult;
 import org.cryptomator.cryptofs.health.api.HealthCheck;
 
 import javafx.beans.Observable;
@@ -23,6 +24,7 @@ public class Check {
 	private final BooleanProperty chosenForExecution = new SimpleBooleanProperty(false);
 	private final ObjectProperty<CheckState> state = new SimpleObjectProperty<>(CheckState.RUNNABLE);
 	private final ObservableList<Result> results = FXCollections.observableArrayList(Result::observables);
+	private final ObjectProperty<DiagnosticResult.Severity> highestResultSeverity = new SimpleObjectProperty<>(null);
 	private final ObjectProperty<Throwable> error = new SimpleObjectProperty<>(null);
 	private final BooleanBinding isInReRunState = state.isNotEqualTo(CheckState.RUNNING).or(state.isNotEqualTo(CheckState.SCHEDULED));
 
@@ -75,6 +77,18 @@ public class Check {
 		error.set(t);
 	}
 
+	ObjectProperty highestResultSeverityProperty() {
+		return highestResultSeverity;
+	}
+
+	DiagnosticResult.Severity getHighestResultSeverity() {
+		return highestResultSeverity.get();
+	}
+
+	void setHighestResultSeverity(DiagnosticResult.Severity severity) {
+		highestResultSeverity.set(severity);
+	}
+
 	boolean isInReRunState() {
 		return isInReRunState.get();
 	}
@@ -83,9 +97,7 @@ public class Check {
 		RUNNABLE,
 		SCHEDULED,
 		RUNNING,
-		WITH_CRITICALS, //TODO: maybe the highest result represnt by property and only use one state
-		WITH_WARNINGS,
-		ALL_GOOD,
+		SUCCEEDED,
 		SKIPPED,
 		ERROR,
 		CANCELLED;

+ 1 - 1
src/main/java/org/cryptomator/ui/health/CheckDetailController.java

@@ -46,7 +46,7 @@ public class CheckDetailController implements FxController {
 		this.checkRunning = checkState.map(Check.CheckState.RUNNING::equals).orElse(false);
 		this.checkScheduled = checkState.map(Check.CheckState.SCHEDULED::equals).orElse(false);
 		this.checkSkipped = checkState.map(Check.CheckState.SKIPPED::equals).orElse(false);
-		this.checkSucceeded = checkState.map(state -> state == Check.CheckState.ALL_GOOD || state == Check.CheckState.WITH_WARNINGS || state == Check.CheckState.WITH_CRITICALS).orElse(false);
+		this.checkSucceeded = checkState.map(Check.CheckState.SUCCEEDED::equals).orElse(false);
 		this.checkFailed = checkState.map(Check.CheckState.ERROR::equals).orElse(false);
 		this.checkCancelled = checkState.map(Check.CheckState.CANCELLED::equals).orElse(false);
 		this.checkFinished = EasyBind.combine(checkSucceeded, checkFailed, checkCancelled, (a, b, c) -> a || b || c);

+ 63 - 79
src/main/java/org/cryptomator/ui/health/CheckExecutor.java

@@ -1,28 +1,21 @@
 package org.cryptomator.ui.health;
 
-import com.google.common.base.Preconditions;
 import org.cryptomator.common.vaults.Vault;
 import org.cryptomator.cryptofs.VaultConfig;
 import org.cryptomator.cryptofs.health.api.DiagnosticResult;
-import org.cryptomator.cryptofs.health.api.HealthCheck;
 import org.cryptomator.cryptolib.api.CryptorProvider;
 import org.cryptomator.cryptolib.api.Masterkey;
 
 import javax.inject.Inject;
-import javafx.application.Platform;
+import javafx.concurrent.Task;
 import java.nio.file.Path;
 import java.security.SecureRandom;
-import java.util.EnumSet;
+import java.util.ArrayDeque;
 import java.util.List;
-import java.util.Set;
-import java.util.concurrent.CancellationException;
-import java.util.concurrent.CompletableFuture;
-import java.util.concurrent.CompletionException;
-import java.util.concurrent.CompletionStage;
+import java.util.Queue;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.atomic.AtomicReference;
-import java.util.function.Consumer;
 
 @HealthCheckScoped
 public class CheckExecutor {
@@ -32,8 +25,7 @@ public class CheckExecutor {
 	private final Masterkey masterkey;
 	private final VaultConfig vaultConfig;
 	private final ExecutorService sequentialExecutor;
-
-	private volatile boolean isCanceled;
+	private final Queue<CheckTask> runningTasks;
 
 
 	@Inject
@@ -43,87 +35,79 @@ public class CheckExecutor {
 		this.vaultConfig = vaultConfigRef.get();
 		this.csprng = csprng;
 		this.sequentialExecutor = Executors.newSingleThreadExecutor();
+		this.runningTasks = new ArrayDeque<>();
 	}
 
-	public synchronized CompletionStage<Void> executeBatch(List<Check> checks) {
-		isCanceled = false;
-		var scheduledChecks = checks.stream().map(this::execute).toArray(CompletableFuture[]::new);
-		return CompletableFuture.allOf(scheduledChecks);
+	public synchronized void executeBatch(List<Check> checks) {
+		checks.stream().map(c -> {
+			c.setState(Check.CheckState.SCHEDULED);
+			var task = new CheckTask(c);
+			runningTasks.add(task); // we need to use CheckTask and not Futures to set state to CANCEL
+			return task;
+		}).forEach(sequentialExecutor::submit);
 	}
 
-	//@formatter:off
-	private CompletionStage<Void> execute(Check check) {
-		Preconditions.checkArgument(check.isInReRunState());
-		return CompletableFuture.runAsync(() -> check.setState(Check.CheckState.SCHEDULED), Platform::runLater)
-			.thenApplyAsync(ignored -> {
-					if (isCanceled) {
-						throw new CancellationException();
-					}
-					Platform.runLater(() -> check.setState(Check.CheckState.RUNNING)); //must be set within the lambda
-					var seenSeverities = EnumSet.noneOf(DiagnosticResult.Severity.class); //used due to efficiency and compactness
-					check(check.getHealthCheck(), diagnosis -> {
-						seenSeverities.add(diagnosis.getSeverity());
-						Platform.runLater(() -> check.getResults().add(Result.create(diagnosis))); //observableLists need to be changed on FXThread
-						if (isCanceled) {
-							throw new CancellationException(); //hacky workaround to stop the check. DO NOT catch this exception (might be wrapped!)
-						}
-					});
-					return determineHighesSeverity(seenSeverities); },
-				sequentialExecutor)
-			.handleAsync((maxSeenSeverity, throwable) -> {
-					var endState = determineEndState(maxSeenSeverity,throwable);
-					check.setState(endState);
-					if( endState != Check.CheckState.CANCELLED) { //canceling throws exception
-						check.setError(throwable);
-					}
-					return null; },
-				Platform::runLater);
-	}
-	//@formatter:on
-
-	private DiagnosticResult.Severity determineHighesSeverity(Set<DiagnosticResult.Severity> seenSeverities) {
-		if (seenSeverities.contains(DiagnosticResult.Severity.CRITICAL)) {
-			return DiagnosticResult.Severity.CRITICAL;
-		} else if (seenSeverities.contains(DiagnosticResult.Severity.WARN)) {
-			return DiagnosticResult.Severity.WARN;
-		} else {
-			return DiagnosticResult.Severity.GOOD;
+	public synchronized void cancel() {
+		while (!runningTasks.isEmpty()) {
+			runningTasks.remove().cancel(true);
 		}
 	}
 
-	private Check.CheckState determineEndState(DiagnosticResult.Severity severity, Throwable t) {
-		if (isCanceled) {
-			//we do not check any exception, because CancellationExc might be wrapped
-			return Check.CheckState.CANCELLED;
-		} else if (t != null) {
-			return Check.CheckState.ERROR;
-		} else if (severity == DiagnosticResult.Severity.GOOD) {
-			return Check.CheckState.ALL_GOOD;
-		} else if (severity == DiagnosticResult.Severity.WARN) {
-			return Check.CheckState.WITH_WARNINGS;
-		} else {
-			return Check.CheckState.WITH_CRITICALS;
+	private class CheckTask extends Task<Void> {
+
+		private Check c;
+		private DiagnosticResult.Severity highestResultSeverity;
+
+		CheckTask(Check c) {
+			this.c = c;
 		}
-	}
 
-	private void check(HealthCheck healthCheck, Consumer<DiagnosticResult> diagnosisConsumer) {
-		try (var masterkeyClone = masterkey.clone(); //
-			 var cryptor = CryptorProvider.forScheme(vaultConfig.getCipherCombo()).provide(masterkeyClone, csprng)) {
-			healthCheck.check(vaultPath, vaultConfig, masterkeyClone, cryptor, diagnosisConsumer);
-		} catch (Exception e) {
-			throw new CheckFailedException(e);
+		@Override
+		protected Void call() throws Exception {
+			try (var masterkeyClone = masterkey.clone(); //
+				 var cryptor = CryptorProvider.forScheme(vaultConfig.getCipherCombo()).provide(masterkeyClone, csprng)) {
+				c.getHealthCheck().check(vaultPath, vaultConfig, masterkeyClone, cryptor, diagnosis -> {
+					c.getResults().add(Result.create(diagnosis));
+					compareAndSetSeverity(diagnosis.getSeverity());
+				});
+			}
+			return null;
 		}
-	}
 
-	public synchronized void cancel() {
-		isCanceled = true;
-	}
+		private void compareAndSetSeverity(DiagnosticResult.Severity newOne) {
+			if (highestResultSeverity != DiagnosticResult.Severity.CRITICAL && newOne == DiagnosticResult.Severity.CRITICAL) {
+				highestResultSeverity = DiagnosticResult.Severity.CRITICAL;
+			} else if (highestResultSeverity != DiagnosticResult.Severity.WARN && newOne == DiagnosticResult.Severity.WARN) {
+				highestResultSeverity = DiagnosticResult.Severity.WARN;
+			} else if (highestResultSeverity != DiagnosticResult.Severity.GOOD && newOne == DiagnosticResult.Severity.GOOD) {
+				highestResultSeverity = DiagnosticResult.Severity.GOOD;
+			} else {
+				highestResultSeverity = DiagnosticResult.Severity.INFO;
+			}
+		}
+
+		@Override
+		protected void running() {
+			c.setState(Check.CheckState.RUNNING);
+		}
 
-	public static class CheckFailedException extends CompletionException {
+		@Override
+		protected void cancelled() {
+			c.setState(Check.CheckState.CANCELLED);
+		}
+
+		@Override
+		protected void succeeded() {
+			c.setState(Check.CheckState.SUCCEEDED);
+			c.setHighestResultSeverity(highestResultSeverity);
+		}
 
-		private CheckFailedException(Throwable cause) {
-			super(cause);
+		@Override
+		protected void failed() {
+			c.setState(Check.CheckState.ERROR);
+			c.setError(this.getException());
 		}
+
 	}
 
 }

+ 9 - 3
src/main/java/org/cryptomator/ui/health/CheckListCell.java

@@ -1,6 +1,7 @@
 package org.cryptomator.ui.health;
 
 import com.tobiasdiez.easybind.EasyBind;
+import org.cryptomator.cryptofs.health.api.DiagnosticResult;
 import org.cryptomator.ui.controls.FontAwesome5Icon;
 import org.cryptomator.ui.controls.FontAwesome5IconView;
 
@@ -34,7 +35,7 @@ class CheckListCell extends ListCell<Check> {
 		if (item != null) {
 			setText(item.getLocalizedName());
 			graphicProperty().bind(EasyBind.map(item.stateProperty(),this::chooseNodeFromState));
-			stateIcon.glyphProperty().bind(Bindings.createObjectBinding(() -> glyphForState(item), item.stateProperty()));
+			stateIcon.glyphProperty().bind(Bindings.createObjectBinding(() -> glyphForState(item), item.stateProperty(), item.highestResultSeverityProperty()));
 			checkBox.selectedProperty().bindBidirectional(item.chosenForExecutionProperty());
 		} else {
 			graphicProperty().unbind();
@@ -60,8 +61,13 @@ class CheckListCell extends ListCell<Check> {
 			case RUNNING -> FontAwesome5Icon.SPINNER;
 			case ERROR -> FontAwesome5Icon.TIMES;
 			case CANCELLED -> FontAwesome5Icon.BAN;
-			case ALL_GOOD -> FontAwesome5Icon.CHECK;
-			case WITH_WARNINGS, WITH_CRITICALS -> FontAwesome5Icon.EXCLAMATION_TRIANGLE;
+			case SUCCEEDED -> {
+				if (item.getHighestResultSeverity() == DiagnosticResult.Severity.INFO || item.getHighestResultSeverity() == DiagnosticResult.Severity.GOOD) {
+					yield FontAwesome5Icon.CHECK;
+				} else {
+					yield FontAwesome5Icon.EXCLAMATION_TRIANGLE;
+				}
+			}
 		};
 	}
 

+ 1 - 1
src/main/java/org/cryptomator/ui/health/ReportWriter.java

@@ -64,7 +64,7 @@ public class ReportWriter {
 			for (var check : performedChecks) {
 				writer.write(REPORT_CHECK_HEADER.formatted(check.getHealthCheck().identifier()));
 				switch (check.getState()) {
-					case ALL_GOOD, WITH_CRITICALS, WITH_WARNINGS -> {
+					case SUCCEEDED -> {
 						writer.write("STATUS: SUCCESS\nRESULTS:\n");
 						for (var result : check.getResults()) {
 							writer.write(REPORT_CHECK_RESULT.formatted(result.diagnosis().getSeverity(), result.getDescription()));

+ 2 - 2
src/main/resources/fxml/health_check_details.fxml

@@ -18,7 +18,7 @@
 	<Label text="%health.check.detail.checkFailed" visible="${controller.checkFailed}" managed="${controller.checkFailed}"/>
 	<Label text="%health.check.detail.checkSucceeded" visible="${controller.checkSucceeded}" managed="${controller.checkSucceeded}"/>
 
-	<FormattedLabel styleClass="label" format="%health.check.detail.problemCount" arg1="${controller.countOfWarnSeverity}" arg2="${controller.countOfCritSeverity}" visible="${!controller.taskNotStarted}"
-					 managed="${!controller.taskNotStarted}"	/>
+	<FormattedLabel styleClass="label" format="%health.check.detail.problemCount" arg1="${controller.countOfWarnSeverity}" arg2="${controller.countOfCritSeverity}" visible="${!controller.checkSkipped}"
+					 managed="${!controller.checkSkipped}"	/>
 	<ListView fx:id="resultsListView" VBox.vgrow="ALWAYS"/>
 </VBox>