diff --git a/.idea/inspectionProfiles/Project_Default.xml b/.idea/inspectionProfiles/Project_Default.xml index aeb663b6..8cc562db 100644 --- a/.idea/inspectionProfiles/Project_Default.xml +++ b/.idea/inspectionProfiles/Project_Default.xml @@ -33,6 +33,7 @@ + diff --git a/build-logic/src/main/kotlin/pklJSpecify.gradle.kts b/build-logic/src/main/kotlin/pklJSpecify.gradle.kts index a41115ce..e7c9068c 100644 --- a/build-logic/src/main/kotlin/pklJSpecify.gradle.kts +++ b/build-logic/src/main/kotlin/pklJSpecify.gradle.kts @@ -38,6 +38,7 @@ nullaway { onlyNullMarked = true } tasks.withType().configureEach { options.errorprone.disableAllChecks = true options.errorprone.check("StringCaseLocaleUsage", CheckSeverity.ERROR) + options.errorprone.check("GuardedBy", CheckSeverity.ERROR) options.errorprone.nullaway { error() onlyNullMarked = true diff --git a/pkl-core/src/main/java/org/pkl/core/externalreader/ExternalReaderProcessImpl.java b/pkl-core/src/main/java/org/pkl/core/externalreader/ExternalReaderProcessImpl.java index c26e925d..fc5fa8fc 100644 --- a/pkl-core/src/main/java/org/pkl/core/externalreader/ExternalReaderProcessImpl.java +++ b/pkl-core/src/main/java/org/pkl/core/externalreader/ExternalReaderProcessImpl.java @@ -34,7 +34,6 @@ import org.pkl.core.messaging.MessageTransport; import org.pkl.core.messaging.MessageTransports; import org.pkl.core.messaging.ProtocolException; import org.pkl.core.util.ErrorMessages; -import org.pkl.core.util.LateInit; final class ExternalReaderProcessImpl implements ExternalReaderProcess { @@ -51,13 +50,11 @@ final class ExternalReaderProcessImpl implements ExternalReaderProcess { private final Object lock = new Object(); private @GuardedBy("lock") boolean closed = false; - @LateInit @GuardedBy("lock") - private Process process; + private @Nullable Process process; - @LateInit @GuardedBy("lock") - private MessageTransport transport; + private @Nullable MessageTransport transport; private void log(String msg) { if (logPrefix != null) { @@ -96,35 +93,40 @@ final class ExternalReaderProcessImpl implements ExternalReaderProcess { ErrorMessages.create("externalReaderAlreadyTerminated")); } + assert transport != null; return transport; } + + // This relies on Java/OS behavior around PATH resolution, absolute/relative paths, etc. + var command = new ArrayList(); + command.add(spec.executable()); + if (spec.arguments() != null) { + command.addAll(spec.arguments()); + } + + var builder = new ProcessBuilder(command); + builder.redirectError(Redirect.INHERIT); // inherit stderr from this pkl process + try { + process = builder.start(); + } catch (IOException e) { + throw new ExternalReaderProcessException(e); + } + transport = + MessageTransports.stream( + new ExternalReaderMessagePackDecoder(process.getInputStream()), + new ExternalReaderMessagePackEncoder(process.getOutputStream()), + this::log); + + var myTransport = transport; + var rxThread = + new Thread( + () -> this.runTransport(myTransport), + "ExternalReaderProcessImpl rxThread for " + spec); + rxThread.setDaemon(true); + rxThread.start(); + + return transport; } - - // This relies on Java/OS behavior around PATH resolution, absolute/relative paths, etc. - var command = new ArrayList(); - command.add(spec.executable()); - if (spec.arguments() != null) { - command.addAll(spec.arguments()); - } - - var builder = new ProcessBuilder(command); - builder.redirectError(Redirect.INHERIT); // inherit stderr from this pkl process - try { - process = builder.start(); - } catch (IOException e) { - throw new ExternalReaderProcessException(e); - } - transport = - MessageTransports.stream( - new ExternalReaderMessagePackDecoder(process.getInputStream()), - new ExternalReaderMessagePackEncoder(process.getOutputStream()), - this::log); - - var rxThread = new Thread(this::runTransport, "ExternalReaderProcessImpl rxThread for " + spec); - rxThread.setDaemon(true); - rxThread.start(); - - return transport; } /** @@ -132,7 +134,7 @@ final class ExternalReaderProcessImpl implements ExternalReaderProcess { * *

Blocks until the underlying transport is closed. */ - private void runTransport() { + private void runTransport(MessageTransport transport) { try { transport.start( (msg) -> { diff --git a/pkl-core/src/main/java/org/pkl/core/module/ModulePathResolver.java b/pkl-core/src/main/java/org/pkl/core/module/ModulePathResolver.java index 6fdd6764..2b8ab8b2 100644 --- a/pkl-core/src/main/java/org/pkl/core/module/ModulePathResolver.java +++ b/pkl-core/src/main/java/org/pkl/core/module/ModulePathResolver.java @@ -65,6 +65,7 @@ public final class ModulePathResolver implements AutoCloseable { this.modulePath = modulePath; } + @GuardedBy("lock") private void populateCaches() throws IOException { fileCache = new HashMap<>(); zipFileSystems = new ArrayList<>(); @@ -139,30 +140,29 @@ public final class ModulePathResolver implements AutoCloseable { } } + @GuardedBy("lock") private void populateFileCache(Path basePath) throws IOException { try (var stream = Files.find( basePath, Integer.MAX_VALUE, // reduce file cache size by filtering out .class files - // (currently `read()` only supports text resources, no known use case for accessing - // .class files from Pkl code) + // (no known use case for accessing .class files from Pkl code) (path, attributes) -> attributes.isRegularFile() && !path.toString().endsWith(".class"))) { // in case of duplicate path, first entry wins (cf. class loader) - stream.forEach( - (path) -> { - var relativized = IoUtils.relativize(path, basePath); - assert fileCache != null; - fileCache.putIfAbsent(IoUtils.toNormalizedPathString(relativized), path); - assert cachedPathElementRoot != null; - var element = cachedPathElementRoot; - for (var i = 0; i < relativized.getNameCount(); i++) { - var name = relativized.getName(i).toString(); - var isDirectory = i < (relativized.getNameCount() - 1); - element = element.putIfAbsent(name, new TreePathElement(name, isDirectory)); - } - }); + for (var path : stream.toList()) { + var relativized = IoUtils.relativize(path, basePath); + assert fileCache != null; + fileCache.putIfAbsent(IoUtils.toNormalizedPathString(relativized), path); + assert cachedPathElementRoot != null; + var element = cachedPathElementRoot; + for (var i = 0; i < relativized.getNameCount(); i++) { + var name = relativized.getName(i).toString(); + var isDirectory = i < (relativized.getNameCount() - 1); + element = element.putIfAbsent(name, new TreePathElement(name, isDirectory)); + } + } } } diff --git a/pkl-core/src/main/java/org/pkl/core/module/ProjectDependenciesManager.java b/pkl-core/src/main/java/org/pkl/core/module/ProjectDependenciesManager.java index c9a58ec6..913524f7 100644 --- a/pkl-core/src/main/java/org/pkl/core/module/ProjectDependenciesManager.java +++ b/pkl-core/src/main/java/org/pkl/core/module/ProjectDependenciesManager.java @@ -96,6 +96,7 @@ public final class ProjectDependenciesManager { } } + @GuardedBy("lock") private void ensureLocalProjectDependencyInitialized( DeclaredDependencies localProjectDependencies, ProjectDeps projectDeps) { // turn `package:` scheme into `projectpackage`: scheme @@ -162,17 +163,23 @@ public final class ProjectDependenciesManager { return ret; } + // `ensureDependenciesInitialized` makes `myDependencies` safe to access + @SuppressWarnings({"FieldAccessNotGuarded", "GuardedBy"}) public Map getDependencies() { ensureDependenciesInitialized(); assert myDependencies != null; return myDependencies; } + // `ensureDependenciesInitialized` makes `localPackageDependencies` safe to access + @SuppressWarnings({"FieldAccessNotGuarded", "GuardedBy"}) public boolean isLocalPackage(PackageUri packageUri) { ensureDependenciesInitialized(); return localPackageDependencies.containsKey(packageUri); } + // `ensureDependenciesInitialized` makes `localPackageDependencies` safe to access + @SuppressWarnings({"FieldAccessNotGuarded", "GuardedBy"}) public Map getLocalPackageDependencies(PackageUri packageUri) { ensureDependenciesInitialized(); var dep = localPackageDependencies.get(packageUri); diff --git a/pkl-core/src/main/java/org/pkl/core/packages/PackageResolvers.java b/pkl-core/src/main/java/org/pkl/core/packages/PackageResolvers.java index b8c8f52a..c6ffbff7 100644 --- a/pkl-core/src/main/java/org/pkl/core/packages/PackageResolvers.java +++ b/pkl-core/src/main/java/org/pkl/core/packages/PackageResolvers.java @@ -87,6 +87,8 @@ final class PackageResolvers { checkNotClosed(); synchronized (lock) { var metadata = cachedDependencyMetadata.get(uri); + // incorrect "condition is always false" inspection + //noinspection ConstantValue if (metadata == null) { metadata = doGetDependencyMetadata(uri, checksums); cachedDependencyMetadata.put(uri, metadata); @@ -318,7 +320,10 @@ final class PackageResolvers { throws IOException, SecurityManagerException { var packageUri = uri.getPackageUri(); ensurePackageDownloaded(packageUri, checksums); - var elem = cachedTreePathElementRoots.get(uri.getPackageUri()).getElement(uri.getAssetPath()); + TreePathElement elem; + synchronized (lock) { + elem = cachedTreePathElementRoots.get(uri.getPackageUri()).getElement(uri.getAssetPath()); + } if (elem == null) { throw new FileNotFoundException(); } else if (elem.isDirectory()) { @@ -332,7 +337,10 @@ final class PackageResolvers { } throw fileIsADirectory(); } - var entries = cachedEntries.get(packageUri); + EconomicMap entries; + synchronized (lock) { + entries = cachedEntries.get(packageUri); + } // need to normalize here but not in `doListElements` nor `doHasElement` because // `TreePathElement.getElement` does normalization already. var path = IoUtils.toNormalizedPathString(Path.of(uri.getAssetPath()).normalize()); @@ -344,7 +352,10 @@ final class PackageResolvers { throws IOException, SecurityManagerException { var packageUri = uri.getPackageUri(); ensurePackageDownloaded(packageUri, checksums); - var element = cachedTreePathElementRoots.get(packageUri).getElement(uri.getAssetPath()); + TreePathElement element; + synchronized (lock) { + element = cachedTreePathElementRoots.get(packageUri).getElement(uri.getAssetPath()); + } if (element == null) { return Collections.emptyList(); } @@ -356,8 +367,10 @@ final class PackageResolvers { throws IOException, SecurityManagerException { var packageUri = uri.getPackageUri(); ensurePackageDownloaded(packageUri, checksums); - var element = cachedTreePathElementRoots.get(packageUri).getElement(uri.getAssetPath()); - return element != null; + synchronized (lock) { + var element = cachedTreePathElementRoots.get(packageUri).getElement(uri.getAssetPath()); + return element != null; + } } @Override @@ -570,6 +583,8 @@ final class PackageResolvers { var packageUri = uri.getPackageUri(); synchronized (lock) { var fs = fileSystems.get(packageUri); + // incorrect "condition is always false" inspection + //noinspection ConstantValue if (fs == null) { var metadata = getDependencyMetadata(packageUri, checksums); var zipFilePath = getZipFilePath(packageUri, metadata); diff --git a/pkl-core/src/main/java/org/pkl/core/runtime/VmClass.java b/pkl-core/src/main/java/org/pkl/core/runtime/VmClass.java index 14db5a83..2cad2146 100644 --- a/pkl-core/src/main/java/org/pkl/core/runtime/VmClass.java +++ b/pkl-core/src/main/java/org/pkl/core/runtime/VmClass.java @@ -156,8 +156,12 @@ public final class VmClass extends VmValue { EconomicMaps.put(declaredProperties, property.getName(), property); if (!property.isLocal()) { - __allProperties = null; - __allHiddenPropertyNames = null; + synchronized (allPropertiesLock) { + synchronized (allHiddenPropertyNamesLock) { + __allProperties = null; + __allHiddenPropertyNames = null; + } + } } } @@ -173,7 +177,9 @@ public final class VmClass extends VmValue { EconomicMaps.put(declaredMethods, method.getName(), method); if (!method.isLocal()) { - __allMethods = null; + synchronized (allMethodsLock) { + __allMethods = null; + } } } @@ -471,6 +477,7 @@ public final class VmClass extends VmValue { public VmTyped getMirror() { synchronized (mirrorLock) { + //noinspection ConstantValue if (__mirror == null) { __mirror = MirrorFactories.classFactory.create(this); } @@ -481,6 +488,7 @@ public final class VmClass extends VmValue { @TruffleBoundary public EconomicMap getTypedToDynamicMembers() { synchronized (typedToDynamicMembersLock) { + //noinspection ConstantValue if (__typedToDynamicMembers == null) { __typedToDynamicMembers = createDelegatingMembers( @@ -495,6 +503,7 @@ public final class VmClass extends VmValue { @TruffleBoundary public EconomicMap getDynamicToTypedMembers() { synchronized (dynamicToTypedMembersLock) { + //noinspection ConstantValue if (__dynamicToTypedMembers == null) { __dynamicToTypedMembers = createDelegatingMembers( @@ -512,6 +521,7 @@ public final class VmClass extends VmValue { @TruffleBoundary public EconomicMap getMapToTypedMembers() { synchronized (mapToTypedMembersLock) { + //noinspection ConstantValue if (__mapToTypedMembers == null) { __mapToTypedMembers = createDelegatingMembers( @@ -610,6 +620,7 @@ public final class VmClass extends VmValue { @TruffleBoundary public PClass export() { synchronized (pClassLock) { + //noinspection ConstantValue if (__pClass == null) { var exportedAnnotations = new ArrayList(); var properties =