Fix temporary workspace deletion on JVM shutdown (#967)

Author: fvgh

_ext/eclipse-base/CHANGES.md

@@ -3,6 +3,8 @@
 We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (starting after version `3.2.1`).
 
 ## [Unreleased]
+### Fixed
+* Racing condition when cleaning up temporary workspace on JVM runtime shutdown (see [#967](https://github.com/diffplug/spotless/issues/967)). Can lead to error logs and remaining files in workspace.
 
 ## [3.5.0] - 2021-06-20
 ### Added

_ext/eclipse-base/src/main/java/com/diffplug/spotless/extra/eclipse/base/SpotlessEclipseFramework.java

@@ -222,6 +222,14 @@ public void activatePlugins(SpotlessEclipsePluginConfig config) {
 	public synchronized static void setup(SpotlessEclipseConfig config) throws BundleException {
 		if (null == INSTANCE) {
 			INSTANCE = new SpotlessEclipseFramework();
+			Runtime.getRuntime().addShutdownHook(new Thread(() -> {
+				try {
+					INSTANCE.shutdown();
+				} catch (Exception e) {
+					//At shutdown everything is just done on best-efforts basis
+				}
+			}));
+
 			config.registerServices(INSTANCE.getServiceConfig());
 
 			SpotlessEclipseCoreConfig coreConfig = new SpotlessEclipseCoreConfig();
@@ -252,6 +260,10 @@ private SpotlessEclipseFramework() throws BundleException {
 		};
 	}
 
+	void shutdown() {
+		controller.shutdown();
+	}
+
 	private SpotlessEclipseServiceConfig getServiceConfig() {
 		return controller.getServices();
 	}

_ext/eclipse-base/src/main/java/com/diffplug/spotless/extra/eclipse/base/osgi/BundleController.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2016 DiffPlug
+ * Copyright 2016-2021 DiffPlug
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -80,6 +80,17 @@ public BundleController() throws BundleException {
 		FrameworkBundleRegistry.initialize(this);
 	}
 
+	public void shutdown() {
+		bundles.getAll().forEach(b -> {
+			try {
+				b.stop();
+			} catch (IllegalStateException | BundleException e) {
+				//Stop on best effort basis
+			}
+		});
+		services.stop();
+	}
+
 	public ServiceCollection getServices() {
 		return services;
 	}

_ext/eclipse-base/src/main/java/com/diffplug/spotless/extra/eclipse/base/osgi/ServiceCollection.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2016-2020 DiffPlug
+ * Copyright 2016-2021 DiffPlug
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -15,10 +15,12 @@
  */
 package com.diffplug.spotless.extra.eclipse.base.osgi;
 
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Dictionary;
 import java.util.HashMap;
 import java.util.Hashtable;
+import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 
@@ -33,11 +35,15 @@
 
 /**
  * Collection of services.
+ * Eclipse service are not expected to hold any resources. Spotless services
+ * can implement AutoCloseable in case a resource release is required on shutdown.
+ *
  * Note that the collection access is not thread save, since it is expected
  * that the collection is completed before starting any bundles.
  */
 public class ServiceCollection implements SpotlessEclipseServiceConfig {
 	private final Map<String, ServiceReference<?>> className2Service;
+	private final List<AutoCloseable> servicesWithResources;
 	private final Bundle systemBundle;
 	private final Map<String, String> properties;
 
@@ -48,10 +54,21 @@ public class ServiceCollection implements SpotlessEclipseServiceConfig {
 	 */
 	ServiceCollection(Bundle systemBundle, Map<String, String> properties) {
 		className2Service = new HashMap<String, ServiceReference<?>>();
+		servicesWithResources = new ArrayList<>();
 		this.systemBundle = systemBundle;
 		this.properties = properties;
 	}
 
+	void stop() {
+		servicesWithResources.stream().forEach(s -> {
+			try {
+				s.close();
+			} catch (Exception e) {
+				//Stop on best effort basis
+			}
+		});
+	}
+
 	@Override
 	public void set(String key, String value) {
 		properties.put(key, value);
@@ -64,6 +81,9 @@ public <S> void add(Class<S> interfaceClass, S service) throws ServiceException
 			throw new ServiceException(
 					String.format("Service '%s' is already registered.", interfaceClass.getName()), ServiceException.FACTORY_ERROR);
 		}
+		if (service instanceof AutoCloseable) {
+			servicesWithResources.add((AutoCloseable) service);
+		}
 	}
 
 	/** Creates filter object suitable to lookup service by its interface name. */

_ext/eclipse-base/src/main/java/com/diffplug/spotless/extra/eclipse/base/service/TemporaryLocation.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2016 DiffPlug
+ * Copyright 2016-2021 DiffPlug
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -28,7 +28,7 @@
 import org.eclipse.osgi.service.datalocation.Location;
 
 /** All files generated at runtime are stored in a temporary location. */
-public class TemporaryLocation implements Location {
+public class TemporaryLocation implements Location, AutoCloseable {
 	private static final String TEMP_PREFIX = "com_diffplug_spotless_extra_eclipse";
 	private final URL location;
 	private Location parent;
@@ -45,27 +45,12 @@ private TemporaryLocation(Location parent, URL defaultValue) {
 	private static URL createTemporaryDirectory() {
 		try {
 			Path location = Files.createTempDirectory(TEMP_PREFIX);
-			deleteDirectoryRecursivelyOnExit(location);
-			location.toFile().deleteOnExit();
 			return location.toUri().toURL();
 		} catch (IOException e) {
 			throw new IOError(e);
 		}
 	}
 
-	private static void deleteDirectoryRecursivelyOnExit(Path location) {
-		Runtime.getRuntime().addShutdownHook(new Thread(() -> {
-			try {
-				Files.walk(location)
-						.sorted(Comparator.reverseOrder())
-						.map(Path::toFile)
-						.forEach(File::delete);
-			} catch (IOException e) {
-				//At shutdown everything is just done on best-efforts basis
-			}
-		}));
-	}
-
 	@Override
 	public boolean allowsDefault() {
 		return false;
@@ -142,4 +127,18 @@ public URL getDataArea(String path) throws IOException {
 		}
 	}
 
+	@Override
+	public void close() throws Exception {
+		try {
+			Path path = Path.of(location.toURI());
+			Files.walk(path)
+					.sorted(Comparator.reverseOrder())
+					.map(Path::toFile)
+					.forEach(File::delete);
+			path.toFile().delete();
+		} catch (IOException e) {
+			//At shutdown everything is just done on best-efforts basis
+		}
+	}
+
 }