Skip to content

Commit

Permalink
Fixes #2408
Browse files Browse the repository at this point in the history
Refactor CatchingScheduledExecutor to only execute one-shot tasks and use Timer for reoccuring classes.
  • Loading branch information
infeo committed Aug 31, 2022
1 parent 86ed0d3 commit 7b265a1
Show file tree
Hide file tree
Showing 6 changed files with 62 additions and 26 deletions.
20 changes: 19 additions & 1 deletion src/main/java/org/cryptomator/common/CatchingExecutors.java
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import java.util.concurrent.CancellationException;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.Future;
import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.ScheduledThreadPoolExecutor;
import java.util.concurrent.ThreadFactory;
import java.util.concurrent.ThreadPoolExecutor;
Expand All @@ -22,12 +23,28 @@ public final class CatchingExecutors {

private CatchingExecutors() { /* NO-OP */ }

public static class CatchingScheduledThreadPoolExecutor extends ScheduledThreadPoolExecutor {
/**
* Executor suitable for scheduled, <em>one-shot</em> Tasks.
* <p>
* This executor does not support repeated execution, see {@link OneShotScheduledExecutorService}
*/
public static class CatchingScheduledThreadPoolExecutor extends ScheduledThreadPoolExecutor implements OneShotScheduledExecutorService {

public CatchingScheduledThreadPoolExecutor(int corePoolSize, ThreadFactory threadFactory) {
super(corePoolSize, threadFactory);
}

@Override
public ScheduledFuture<?> scheduleAtFixedRate(Runnable command, long initalDelay, long period, TimeUnit unit) {
throw new UnsupportedOperationException("This is an one-shot executor service!");
}

@Override
public ScheduledFuture<?> scheduleWithFixedDelay(Runnable command, long initalDelay, long delay, TimeUnit unit) {
throw new UnsupportedOperationException("This is an one-shot executor service!");
}


@Override
protected void afterExecute(Runnable runnable, Throwable throwable) {
super.afterExecute(runnable, throwable);
Expand Down Expand Up @@ -77,6 +94,7 @@ private static void afterExecuteTask(Task<?> task) {
}

private static void afterExecuteFuture(Future<?> future) {
assert future.isDone(): "Future must be in Done state to extract possible exception";
try {
future.get();
} catch (CancellationException ce) {
Expand Down
5 changes: 2 additions & 3 deletions src/main/java/org/cryptomator/common/CommonsModule.java
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
import java.security.SecureRandom;
import java.util.Comparator;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.SynchronousQueue;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;
Expand Down Expand Up @@ -92,9 +91,9 @@ static Settings provideSettings(SettingsProvider settingsProvider) {

@Provides
@Singleton
static ScheduledExecutorService provideScheduledExecutorService(ShutdownHook shutdownHook) {
static OneShotScheduledExecutorService provideScheduledExecutorService(ShutdownHook shutdownHook) {
final AtomicInteger threadNumber = new AtomicInteger(1);
ScheduledExecutorService executorService = new CatchingExecutors.CatchingScheduledThreadPoolExecutor(NUM_SCHEDULER_THREADS, r -> {
OneShotScheduledExecutorService executorService = new CatchingExecutors.CatchingScheduledThreadPoolExecutor(NUM_SCHEDULER_THREADS, r -> {
String name = String.format("App Scheduled Executor %02d", threadNumber.getAndIncrement());
Thread t = new Thread(r);
t.setName(name);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package org.cryptomator.common;

import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;

/**
* Marker interface to indicate the used {@link ScheduledExecutorService} does not support repeated execution via {@link java.util.concurrent.ScheduledExecutorService#scheduleAtFixedRate(Runnable, long, long, TimeUnit)}} or {@link java.util.concurrent.ScheduledExecutorService#scheduleWithFixedDelay(Runnable, long, long, TimeUnit)}.
* Calling one of those methods must throw an {@link UnsupportedOperationException}.
*/
public interface OneShotScheduledExecutorService extends ScheduledExecutorService {

}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import com.google.gson.JsonParseException;
import com.google.gson.JsonParser;
import org.cryptomator.common.Environment;
import org.cryptomator.common.OneShotScheduledExecutorService;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand All @@ -34,7 +35,6 @@
import java.nio.file.StandardCopyOption;
import java.nio.file.StandardOpenOption;
import java.util.Optional;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicReference;
Expand All @@ -50,11 +50,11 @@ public class SettingsProvider implements Supplier<Settings> {
private final AtomicReference<ScheduledFuture<?>> scheduledSaveCmd = new AtomicReference<>();
private final Supplier<Settings> settings = Suppliers.memoize(this::load);
private final Environment env;
private final ScheduledExecutorService scheduler;
private final OneShotScheduledExecutorService scheduler;
private final Gson gson;

@Inject
public SettingsProvider(SettingsJsonAdapter settingsJsonAdapter, Environment env, ScheduledExecutorService scheduler) {
public SettingsProvider(SettingsJsonAdapter settingsJsonAdapter, Environment env, OneShotScheduledExecutorService scheduler) {
this.env = env;
this.scheduler = scheduler;
this.gson = new GsonBuilder() //
Expand Down
22 changes: 15 additions & 7 deletions src/main/java/org/cryptomator/common/vaults/AutoLocker.java
Original file line number Diff line number Diff line change
Expand Up @@ -7,25 +7,34 @@
import javax.inject.Singleton;
import javafx.collections.ObservableList;
import java.time.Instant;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;
import java.util.Timer;
import java.util.TimerTask;
import java.util.concurrent.ExecutorService;

@Singleton
public class AutoLocker {

private static final Logger LOG = LoggerFactory.getLogger(AutoLocker.class);

private final ScheduledExecutorService scheduler;
private final ExecutorService executor;
private final Timer timer;
private final ObservableList<Vault> vaultList;

@Inject
public AutoLocker(ScheduledExecutorService scheduler, ObservableList<Vault> vaultList) {
this.scheduler = scheduler;
public AutoLocker(ExecutorService executor, ObservableList<Vault> vaultList) {
this.executor = executor;
this.timer = new Timer("AutoLocker.SubmitTickTimer", true);
this.vaultList = vaultList;
}

public void init() {
scheduler.scheduleAtFixedRate(this::tick, 0, 1, TimeUnit.MINUTES);
TimerTask submitTask = new TimerTask() {
@Override
public void run() {
executor.submit(AutoLocker.this::tick);
}
};
timer.scheduleAtFixedRate(submitTask, 0, 1000);
}

private void tick() {
Expand All @@ -46,7 +55,6 @@ private void autolock(Vault vault) {

private boolean exceedsIdleTime(Vault vault) {
assert vault.isUnlocked();
// TODO: shouldn't we read these properties from within FX Application Thread?
if (vault.getVaultSettings().autoLockWhenIdle().get()) {
int maxIdleSeconds = vault.getVaultSettings().autoLockIdleSeconds().get();
var deadline = vault.getStats().getLastActivity().plusSeconds(maxIdleSeconds);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,9 @@
import javafx.scene.control.ContentDisplay;
import javafx.stage.Stage;
import java.util.Arrays;
import java.util.Timer;
import java.util.TimerTask;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.TimeUnit;

import static org.cryptomator.common.Constants.MASTERKEY_FILENAME;
import static org.cryptomator.common.Constants.VAULTCONFIG_FILENAME;
Expand All @@ -55,7 +54,6 @@ public class MigrationRunController implements FxController {
private final Stage window;
private final Vault vault;
private final ExecutorService executor;
private final ScheduledExecutorService scheduler;
private final KeychainManager keychain;
private final ObjectProperty<FileSystemCapabilityChecker.Capability> missingCapability;
private final FxApplicationWindows appWindows;
Expand All @@ -73,11 +71,10 @@ public class MigrationRunController implements FxController {
public NiceSecurePasswordField passwordField;

@Inject
public MigrationRunController(@MigrationWindow Stage window, @MigrationWindow Vault vault, ExecutorService executor, ScheduledExecutorService scheduler, KeychainManager keychain, @Named("capabilityErrorCause") ObjectProperty<FileSystemCapabilityChecker.Capability> missingCapability, @FxmlScene(FxmlFile.MIGRATION_START) Lazy<Scene> startScene, @FxmlScene(FxmlFile.MIGRATION_SUCCESS) Lazy<Scene> successScene, @FxmlScene(FxmlFile.MIGRATION_CAPABILITY_ERROR) Lazy<Scene> capabilityErrorScene, @FxmlScene(FxmlFile.MIGRATION_IMPOSSIBLE) Lazy<Scene> impossibleScene, FxApplicationWindows appWindows) {
public MigrationRunController(@MigrationWindow Stage window, @MigrationWindow Vault vault, ExecutorService executor, KeychainManager keychain, @Named("capabilityErrorCause") ObjectProperty<FileSystemCapabilityChecker.Capability> missingCapability, @FxmlScene(FxmlFile.MIGRATION_START) Lazy<Scene> startScene, @FxmlScene(FxmlFile.MIGRATION_SUCCESS) Lazy<Scene> successScene, @FxmlScene(FxmlFile.MIGRATION_CAPABILITY_ERROR) Lazy<Scene> capabilityErrorScene, @FxmlScene(FxmlFile.MIGRATION_IMPOSSIBLE) Lazy<Scene> impossibleScene, FxApplicationWindows appWindows) {
this.window = window;
this.vault = vault;
this.executor = executor;
this.scheduler = scheduler;
this.keychain = keychain;
this.missingCapability = missingCapability;
this.appWindows = appWindows;
Expand Down Expand Up @@ -113,11 +110,13 @@ public void migrate() {
CharSequence password = passwordField.getCharacters();
vault.stateProperty().transition(VaultState.Value.NEEDS_MIGRATION, VaultState.Value.PROCESSING);
passwordField.setDisable(true);
ScheduledFuture<?> progressSyncTask = scheduler.scheduleAtFixedRate(() -> {
Platform.runLater(() -> {
migrationProgress.set(volatileMigrationProgress);
});
}, 0, MIGRATION_PROGRESS_UPDATE_MILLIS, TimeUnit.MILLISECONDS);
Timer timer = new Timer("Migration Timer", true);
timer.scheduleAtFixedRate(new TimerTask() {
@Override
public void run() {
Platform.runLater(() -> migrationProgress.set(volatileMigrationProgress));
}
}, 0, MIGRATION_PROGRESS_UPDATE_MILLIS);
Tasks.create(() -> {
Migrators migrators = Migrators.get();
migrators.migrate(vault.getPath(), VAULTCONFIG_FILENAME, MASTERKEY_FILENAME, password, this::migrationProgressChanged, this::migrationRequiresInput);
Expand Down Expand Up @@ -154,7 +153,7 @@ public void migrate() {
appWindows.showErrorWindow(e, window, startScene.get());
}).andFinally(() -> {
passwordField.setDisable(false);
progressSyncTask.cancel(true);
timer.cancel();
}).runOnce(executor);
}

Expand Down

0 comments on commit 7b265a1

Please sign in to comment.