From adc91faf1df30286786e3b5e3d90e61a2ead3d10 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Fri, 29 May 2020 23:54:46 -0700 Subject: [PATCH 1/6] This instance is thread-specific, so it would have to be a thread-local cache, which is pretty complicated. --- .../com/diffplug/spotless/npm/NpmFormatterStepStateBase.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/src/main/java/com/diffplug/spotless/npm/NpmFormatterStepStateBase.java b/lib/src/main/java/com/diffplug/spotless/npm/NpmFormatterStepStateBase.java index 2ebaa8114a..dd7f22d6ad 100644 --- a/lib/src/main/java/com/diffplug/spotless/npm/NpmFormatterStepStateBase.java +++ b/lib/src/main/java/com/diffplug/spotless/npm/NpmFormatterStepStateBase.java @@ -96,7 +96,7 @@ private File resolveNpm(@Nullable File npm) { } protected NodeJSWrapper nodeJSWrapper() { - return new NodeJSWrapper(this.jarState.getClassLoader()); // TODO (simschla, 02.08.18): cache this instance + return new NodeJSWrapper(this.jarState.getClassLoader()); } protected File nodeModulePath() { From 12c9259219b8789d9860af676c0f2c24261157d3 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Thu, 28 May 2020 18:22:17 -0700 Subject: [PATCH 2/6] Add a test to easily reproduce issue #302. --- .../npm/NpmFormatterStepStateBase.java | 2 +- .../npm/NodeJsNativeDoubleLoadTest.java | 53 +++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) create mode 100644 testlib/src/test/java/com/diffplug/spotless/npm/NodeJsNativeDoubleLoadTest.java diff --git a/lib/src/main/java/com/diffplug/spotless/npm/NpmFormatterStepStateBase.java b/lib/src/main/java/com/diffplug/spotless/npm/NpmFormatterStepStateBase.java index dd7f22d6ad..8b041e3dcb 100644 --- a/lib/src/main/java/com/diffplug/spotless/npm/NpmFormatterStepStateBase.java +++ b/lib/src/main/java/com/diffplug/spotless/npm/NpmFormatterStepStateBase.java @@ -103,7 +103,7 @@ protected File nodeModulePath() { return new File(new File(this.nodeModulesDir, "node_modules"), this.npmConfig.getNpmModule()); } - private String j2v8MavenCoordinate() { + static String j2v8MavenCoordinate() { return "com.eclipsesource.j2v8:j2v8_" + PlatformInfo.normalizedOSName() + "_" + PlatformInfo.normalizedArchName() + ":4.6.0"; } diff --git a/testlib/src/test/java/com/diffplug/spotless/npm/NodeJsNativeDoubleLoadTest.java b/testlib/src/test/java/com/diffplug/spotless/npm/NodeJsNativeDoubleLoadTest.java new file mode 100644 index 0000000000..33ec041cf2 --- /dev/null +++ b/testlib/src/test/java/com/diffplug/spotless/npm/NodeJsNativeDoubleLoadTest.java @@ -0,0 +1,53 @@ +/* + * Copyright 2016 DiffPlug + * + * 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.diffplug.spotless.npm; + +import java.util.Optional; + +import org.assertj.core.api.Assertions; +import org.junit.Test; + +import com.diffplug.common.collect.ImmutableMap; +import com.diffplug.spotless.JarState; +import com.diffplug.spotless.TestProvisioner; + +public class NodeJsNativeDoubleLoadTest { + @Test + public void inMultipleClassLoaders() throws Exception { + JarState state = JarState.from(NpmFormatterStepStateBase.j2v8MavenCoordinate(), TestProvisioner.mavenCentral()); + ClassLoader loader1 = state.getClassLoader(1); + ClassLoader loader2 = state.getClassLoader(2); + createAndTestWrapper(loader1); + createAndTestWrapper(loader2); + } + + @Test + public void multipleTimesInOneClassLoader() throws Exception { + JarState state = JarState.from(NpmFormatterStepStateBase.j2v8MavenCoordinate(), TestProvisioner.mavenCentral()); + ClassLoader loader3 = state.getClassLoader(3); + createAndTestWrapper(loader3); + createAndTestWrapper(loader3); + } + + private void createAndTestWrapper(ClassLoader loader) throws Exception { + try (NodeJSWrapper node = new NodeJSWrapper(loader)) { + V8ObjectWrapper object = node.createNewObject(ImmutableMap.of("a", 1)); + Optional value = object.getOptionalInteger("a"); + Assertions.assertThat(value).hasValue(1); + object.release(); + } + } +} From fb82700187712696139874606147ede3cfb28b8b Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Sat, 30 May 2020 00:00:09 -0700 Subject: [PATCH 3/6] Fix it. --- .../diffplug/spotless/npm/NodeJSWrapper.java | 36 ++++++- .../diffplug/spotless/npm/NodeJsGlobal.java | 93 +++++++++++++++++++ .../com/diffplug/spotless/npm/Reflective.java | 27 ++++++ 3 files changed, 152 insertions(+), 4 deletions(-) create mode 100644 lib/src/main/java/com/diffplug/spotless/npm/NodeJsGlobal.java diff --git a/lib/src/main/java/com/diffplug/spotless/npm/NodeJSWrapper.java b/lib/src/main/java/com/diffplug/spotless/npm/NodeJSWrapper.java index 85cb8467f2..5d487f233c 100644 --- a/lib/src/main/java/com/diffplug/spotless/npm/NodeJSWrapper.java +++ b/lib/src/main/java/com/diffplug/spotless/npm/NodeJSWrapper.java @@ -16,11 +16,13 @@ package com.diffplug.spotless.npm; import java.io.File; +import java.util.HashSet; import java.util.Map; import java.util.Objects; -import java.util.concurrent.atomic.AtomicBoolean; +import java.util.Set; import com.diffplug.spotless.LineEnding; +import com.diffplug.spotless.ThrowingEx; class NodeJSWrapper extends ReflectiveObjectWrapper { @@ -29,15 +31,41 @@ class NodeJSWrapper extends ReflectiveObjectWrapper { public static final String WRAPPED_CLASS = "com.eclipsesource.v8.NodeJS"; - private static final AtomicBoolean flagsSet = new AtomicBoolean(false); + private static final Set alreadySetup = new HashSet<>(); public NodeJSWrapper(ClassLoader classLoader) { super(Reflective.withClassLoader(classLoader), reflective -> { - final boolean firstRun = flagsSet.compareAndSet(false, true); - if (firstRun && LineEnding.PLATFORM_NATIVE.str().equals("\r\n")) { + if (LineEnding.PLATFORM_NATIVE.str().equals("\r\n")) { reflective.invokeStaticMethod(V8_RUNTIME_CLASS, "setFlags", "-color=false"); // required to run prettier on windows } + if (alreadySetup.add(classLoader)) { + // the bridge to node.js needs a .dll/.so/.dylib which gets loaded through System.load + // the problem is that when the JVM loads that DLL, it is bound to the specific classloader that called System.load + // no other classloaders have access to it, and if any other classloader tries to load it, you get an error + // + // ...but, you can copy that DLL as many times as you want, and each classloader can load its own copy of the DLL, and + // that is fine + // + // ...but the in order to do that, we have to manually load the DLL per classloader ourselves, which involves some + // especially hacky reflection into J2V8 *and* JVM internal + // + // so this is bad code, but it fixes our problem, and so far we don't have a better way... + + // here we get the name of the DLL within the jar, and we get our own copy of it on disk + String resource = (String) reflective.invokeStaticMethodPrivate("com.eclipsesource.v8.LibraryLoader", "computeLibraryFullName"); + File file = NodeJsGlobal.sharedLibs.nextDynamicLib(classLoader, resource); + + // ideally, we would call System.load, but the JVM does some tricky stuff to + // figure out who actually called this, and it realizes it was Reflective, which lives + // outside the J2V8 classloader, so System.load doesn't work. Soooo, we have to dig + // into JVM internals and manually tell it "this class from J2V8 called you" + Class libraryLoaderClass = ThrowingEx.get(() -> classLoader.loadClass("com.eclipsesource.v8.LibraryLoader")); + reflective.invokeStaticMethodPrivate("java.lang.ClassLoader", "loadLibrary0", libraryLoaderClass, file); + + // and now we set the flag in J2V8 which says "the DLL is loaded, don't load it again" + reflective.staticFieldPrivate("com.eclipsesource.v8.V8", "nativeLibraryLoaded", true); + } return reflective.invokeStaticMethod(WRAPPED_CLASS, "createNodeJS"); }); } diff --git a/lib/src/main/java/com/diffplug/spotless/npm/NodeJsGlobal.java b/lib/src/main/java/com/diffplug/spotless/npm/NodeJsGlobal.java new file mode 100644 index 0000000000..ad2a5ff428 --- /dev/null +++ b/lib/src/main/java/com/diffplug/spotless/npm/NodeJsGlobal.java @@ -0,0 +1,93 @@ +/* + * Copyright 2016 DiffPlug + * + * 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.diffplug.spotless.npm; + +import java.io.File; +import java.io.FileOutputStream; +import java.io.IOException; +import java.io.InputStream; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Optional; +import java.util.stream.IntStream; + +import com.diffplug.spotless.LineEnding; +import com.diffplug.spotless.ThrowingEx; + +/** Shared config acress the NodeJS steps. */ +public class NodeJsGlobal { + static SharedLibFolder sharedLibs; + + static { + sharedLibs = new SharedLibFolder( + ThrowingEx.get(() -> Files.createTempDirectory("spotless-nodejs"))); + sharedLibs.root.deleteOnExit(); + } + + /** + * All of the NodeJS steps need to extract a bridge DLL for node. By default this is + * a random location, but you can set it to be anywhere. + */ + public static void setSharedLibFolder(File sharedLibFolder) { + sharedLibs = new SharedLibFolder(sharedLibFolder.toPath()); + } + + static class SharedLibFolder { + private final File root; + + private SharedLibFolder(Path root) { + this.root = ThrowingEx.get(() -> root.toFile().getCanonicalFile()); + } + + static final int MAX_CLASSLOADERS_PER_CLEAN = 1_000; + + synchronized File nextDynamicLib(ClassLoader loader, String resource) { + // find a new unique file + Optional nextLibOpt = IntStream.range(0, MAX_CLASSLOADERS_PER_CLEAN) + .mapToObj(i -> new File(root, i + "_" + resource)) + .filter(file -> !file.exists()) + .findFirst(); + if (!nextLibOpt.isPresent()) { + throw new IllegalArgumentException("Overflow, delete the spotless nodeJs cache: " + root); + } + File nextLib = nextLibOpt.get(); + // copy the dll to it + try { + Files.createDirectories(nextLib.getParentFile().toPath()); + try (FileOutputStream fileOut = new FileOutputStream(nextLib); + InputStream resourceIn = loader.loadClass("com.eclipsesource.v8.LibraryLoader").getResourceAsStream("/" + resource)) { + byte[] buf = new byte[0x1000]; + while (true) { + int r = resourceIn.read(buf); + if (r == -1) { + break; + } + fileOut.write(buf, 0, r); + } + } + } catch (IOException | ClassNotFoundException e) { + throw ThrowingEx.asRuntime(e); + } + // make sure it is executable (on unix) + if (LineEnding.PLATFORM_NATIVE.str().equals("\n")) { + ThrowingEx.run(() -> { + Runtime.getRuntime().exec(new String[]{"chmod", "755", nextLib.getAbsolutePath()}).waitFor(); //$NON-NLS-1$ + }); + } + return nextLib; + } + } +} diff --git a/lib/src/main/java/com/diffplug/spotless/npm/Reflective.java b/lib/src/main/java/com/diffplug/spotless/npm/Reflective.java index e1076672b9..3c65ea0492 100644 --- a/lib/src/main/java/com/diffplug/spotless/npm/Reflective.java +++ b/lib/src/main/java/com/diffplug/spotless/npm/Reflective.java @@ -22,6 +22,10 @@ import java.util.Objects; import java.util.StringJoiner; +import com.diffplug.spotless.ThrowingEx; + +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; + class Reflective { private final ClassLoader classLoader; @@ -59,6 +63,17 @@ Object invokeStaticMethod(String className, String methodName, Object... paramet } } + @SuppressFBWarnings("DP_DO_INSIDE_DO_PRIVILEGED") + Object invokeStaticMethodPrivate(String className, String methodName, Object... parameters) { + try { + Method m = staticMethod(className, methodName, parameters); + m.setAccessible(true); + return m.invoke(m.getDeclaringClass(), parameters); + } catch (IllegalAccessException | InvocationTargetException e) { + throw new ReflectiveException(e); + } + } + private Class[] types(TypedValue[] typedValues) { return Arrays.stream(typedValues) .map(TypedValue::getClazz) @@ -215,6 +230,18 @@ private Object staticField(Class clazz, String fieldName) { } } + @SuppressFBWarnings("DP_DO_INSIDE_DO_PRIVILEGED") + void staticFieldPrivate(String className, String fieldName, boolean newValue) { + Class clazz = clazz(className); + try { + Field field = clazz.getDeclaredField(fieldName); + field.setAccessible(true); + field.setBoolean(null, newValue); + } catch (NoSuchFieldException | SecurityException | IllegalArgumentException | IllegalAccessException e) { + throw ThrowingEx.asRuntime(e); + } + } + TypedValue typed(String className, Object obj) { return new TypedValue(clazz(className), obj); } From b64ead5c96d4745f486b08d8c67f5968c7860d92 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Sat, 30 May 2020 00:05:09 -0700 Subject: [PATCH 4/6] For the gradle plugin, put the root files in `{rootProject}/build/spotless-nodejs-cache`. --- .../java/com/diffplug/gradle/spotless/SpotlessExtension.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessExtension.java b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessExtension.java index dd7977aebe..3d683524ad 100644 --- a/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessExtension.java +++ b/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/SpotlessExtension.java @@ -17,6 +17,7 @@ import static java.util.Objects.requireNonNull; +import java.io.File; import java.lang.reflect.Constructor; import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; @@ -31,6 +32,7 @@ import com.diffplug.common.base.Errors; import com.diffplug.spotless.LineEnding; +import com.diffplug.spotless.npm.NodeJsGlobal; public class SpotlessExtension { final Project project; @@ -63,6 +65,8 @@ public SpotlessExtension(Project project) { if (registerDependenciesTask == null) { registerDependenciesTask = project.getRootProject().getTasks().create(RegisterDependenciesTask.TASK_NAME, RegisterDependenciesTask.class); registerDependenciesTask.setup(); + // set where the nodejs runtime will put its temp dlls + NodeJsGlobal.setSharedLibFolder(new File(project.getBuildDir(), "spotless-nodejs-cache")); } this.registerDependenciesTask = registerDependenciesTask; } From c5ae0fb0ba8cc12abd4129cb3ef145e0cda7d573 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Sat, 30 May 2020 00:16:28 -0700 Subject: [PATCH 5/6] It appears we no longer need this on Windows. --- .../main/java/com/diffplug/spotless/npm/NodeJSWrapper.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/lib/src/main/java/com/diffplug/spotless/npm/NodeJSWrapper.java b/lib/src/main/java/com/diffplug/spotless/npm/NodeJSWrapper.java index 5d487f233c..67c121baed 100644 --- a/lib/src/main/java/com/diffplug/spotless/npm/NodeJSWrapper.java +++ b/lib/src/main/java/com/diffplug/spotless/npm/NodeJSWrapper.java @@ -21,7 +21,6 @@ import java.util.Objects; import java.util.Set; -import com.diffplug.spotless.LineEnding; import com.diffplug.spotless.ThrowingEx; class NodeJSWrapper extends ReflectiveObjectWrapper { @@ -36,9 +35,6 @@ class NodeJSWrapper extends ReflectiveObjectWrapper { public NodeJSWrapper(ClassLoader classLoader) { super(Reflective.withClassLoader(classLoader), reflective -> { - if (LineEnding.PLATFORM_NATIVE.str().equals("\r\n")) { - reflective.invokeStaticMethod(V8_RUNTIME_CLASS, "setFlags", "-color=false"); // required to run prettier on windows - } if (alreadySetup.add(classLoader)) { // the bridge to node.js needs a .dll/.so/.dylib which gets loaded through System.load // the problem is that when the JVM loads that DLL, it is bound to the specific classloader that called System.load From 0515d011ace8b350579fe7ba59f8f1d6762b13e4 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Sat, 30 May 2020 00:30:11 -0700 Subject: [PATCH 6/6] Update changelogs. --- CHANGES.md | 4 ++++ plugin-gradle/CHANGES.md | 2 ++ plugin-maven/CHANGES.md | 2 ++ 3 files changed, 8 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index 7ee36a95d4..1fa11be297 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -10,6 +10,10 @@ This document is intended for Spotless developers. We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (starting after version `1.27.0`). ## [Unreleased] +### Added +* `NodeJsGlobal.setSharedLibFolder` allows to set the location of nodejs shared libs. ([#586](https://github.com/diffplug/spotless/pull/586)) +### Fixed +* Previously, the nodejs-based steps would throw `UnsatisfiedLinkError` if they were ever used from more than one classloader. Now they can be used from any number of classloaders (important for gradle build daemon). ([#586](https://github.com/diffplug/spotless/pull/586)) ## [1.31.0] - 2020-05-21 ### Added diff --git a/plugin-gradle/CHANGES.md b/plugin-gradle/CHANGES.md index 88b3ba050f..69a2a5c1d7 100644 --- a/plugin-gradle/CHANGES.md +++ b/plugin-gradle/CHANGES.md @@ -7,6 +7,8 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format ( * Support for ktfmt in KotlinGradleExtension. ([#583](https://github.com/diffplug/spotless/pull/583)) ### Fixed * Users can now run `spotlessCheck` and `spotlessApply` in the same build. ([#584](https://github.com/diffplug/spotless/pull/584)) +* Fixed intermittent `UnsatisfiedLinkError` in nodejs-based steps. ([#586](https://github.com/diffplug/spotless/pull/586)) + * Also, a shared library used by the nodejs steps used to be extracted into the user home directory, but now it is extracted into `{rootProject}/build/spotless-nodejs-cache`. ## [4.0.1] - 2020-05-21 ### Fixed diff --git a/plugin-maven/CHANGES.md b/plugin-maven/CHANGES.md index 247eadaa28..c34bf3702b 100644 --- a/plugin-maven/CHANGES.md +++ b/plugin-maven/CHANGES.md @@ -3,6 +3,8 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (starting after version `1.27.0`). ## [Unreleased] +### Fixed +* Shared library used by the nodejs-based steps used to be extracted into the user home directory, but now it is extracted into a temporary directory and deleted on VM shutdown. ([#586](https://github.com/diffplug/spotless/pull/586)) ## [1.31.1] - 2020-05-21 ### Fixed