From b1170116276a8fa81bec56f9cd4ab8b3151884f9 Mon Sep 17 00:00:00 2001 From: Josh Basch Date: Thu, 31 Oct 2024 14:50:17 -0700 Subject: [PATCH] Refine external reader API * Encapsulate message transport by removing `ExternalReaderProcess.getTransport` and adding `getModuleResolver` and `getResourceResolver` methods * Reuse `Random` instances within `ExternalReaderProcessImpl` and module/resource resolvers * Externalize all `ExternalReaderProcessException` messages * Add some missing doc comments to `ModuleKeyFactories` and `ResourceReaders` methods for external readers * Move org.pkl.core.util.Readers to org.pkl.core.Readers --- .../main/kotlin/org/pkl/cli/CliEvaluator.kt | 2 +- .../kotlin/org/pkl/cli/CliImportAnalyzer.kt | 2 +- .../main/kotlin/org/pkl/cli/CliTestRunner.kt | 2 +- .../pkl/codegen/java/CliJavaCodeGenerator.kt | 2 +- .../codegen/kotlin/CliKotlinCodeGenerator.kt | 2 +- .../java/org/pkl/core/{util => }/Readers.java | 2 +- .../externalreader/ExternalReaderProcess.java | 17 +++++++++--- .../ExternalReaderProcessImpl.java | 25 +++++++++++++++--- .../core/module/ExternalModuleResolver.java | 5 ++-- .../pkl/core/module/ModuleKeyFactories.java | 13 +++++++--- .../resource/ExternalResourceResolver.java | 7 +++-- .../pkl/core/resource/ResourceReaders.java | 26 +++++++++++++------ .../org/pkl/core/service/ExecutorSpiImpl.java | 2 +- .../org/pkl/core/errorMessages.properties | 3 +++ .../TestExternalReaderProcess.kt | 8 +++++- .../kotlin/org/pkl/doc/CliDocGenerator.kt | 2 +- 16 files changed, 89 insertions(+), 31 deletions(-) rename pkl-core/src/main/java/org/pkl/core/{util => }/Readers.java (97%) diff --git a/pkl-cli/src/main/kotlin/org/pkl/cli/CliEvaluator.kt b/pkl-cli/src/main/kotlin/org/pkl/cli/CliEvaluator.kt index 4301c327c..a19220489 100644 --- a/pkl-cli/src/main/kotlin/org/pkl/cli/CliEvaluator.kt +++ b/pkl-cli/src/main/kotlin/org/pkl/cli/CliEvaluator.kt @@ -31,12 +31,12 @@ import org.pkl.commons.writeString import org.pkl.core.EvaluatorBuilder import org.pkl.core.ModuleSource import org.pkl.core.PklException +import org.pkl.core.Readers import org.pkl.core.module.ModulePathResolver import org.pkl.core.runtime.ModuleResolver import org.pkl.core.runtime.VmException import org.pkl.core.runtime.VmUtils import org.pkl.core.util.IoUtils -import org.pkl.core.util.Readers private data class OutputFile(val pathSpec: String, val moduleUri: URI) diff --git a/pkl-cli/src/main/kotlin/org/pkl/cli/CliImportAnalyzer.kt b/pkl-cli/src/main/kotlin/org/pkl/cli/CliImportAnalyzer.kt index 1463216ae..240ad9e67 100644 --- a/pkl-cli/src/main/kotlin/org/pkl/cli/CliImportAnalyzer.kt +++ b/pkl-cli/src/main/kotlin/org/pkl/cli/CliImportAnalyzer.kt @@ -20,7 +20,7 @@ import org.pkl.commons.cli.CliCommand import org.pkl.commons.createParentDirectories import org.pkl.commons.writeString import org.pkl.core.ModuleSource -import org.pkl.core.util.Readers +import org.pkl.core.Readers class CliImportAnalyzer @JvmOverloads diff --git a/pkl-cli/src/main/kotlin/org/pkl/cli/CliTestRunner.kt b/pkl-cli/src/main/kotlin/org/pkl/cli/CliTestRunner.kt index cfead7ab8..defc64d0b 100644 --- a/pkl-cli/src/main/kotlin/org/pkl/cli/CliTestRunner.kt +++ b/pkl-cli/src/main/kotlin/org/pkl/cli/CliTestRunner.kt @@ -19,10 +19,10 @@ import java.io.Writer import org.pkl.commons.cli.* import org.pkl.core.EvaluatorBuilder import org.pkl.core.ModuleSource.uri +import org.pkl.core.Readers import org.pkl.core.stdlib.test.report.JUnitReport import org.pkl.core.stdlib.test.report.SimpleReport import org.pkl.core.util.ErrorMessages -import org.pkl.core.util.Readers class CliTestRunner @JvmOverloads diff --git a/pkl-codegen-java/src/main/kotlin/org/pkl/codegen/java/CliJavaCodeGenerator.kt b/pkl-codegen-java/src/main/kotlin/org/pkl/codegen/java/CliJavaCodeGenerator.kt index 5498448bb..5022d1e04 100644 --- a/pkl-codegen-java/src/main/kotlin/org/pkl/codegen/java/CliJavaCodeGenerator.kt +++ b/pkl-codegen-java/src/main/kotlin/org/pkl/codegen/java/CliJavaCodeGenerator.kt @@ -21,7 +21,7 @@ import org.pkl.commons.cli.CliException import org.pkl.commons.createParentDirectories import org.pkl.commons.writeString import org.pkl.core.ModuleSource -import org.pkl.core.util.Readers +import org.pkl.core.Readers /** API for the Java code generator CLI. */ class CliJavaCodeGenerator(private val options: CliJavaCodeGeneratorOptions) : diff --git a/pkl-codegen-kotlin/src/main/kotlin/org/pkl/codegen/kotlin/CliKotlinCodeGenerator.kt b/pkl-codegen-kotlin/src/main/kotlin/org/pkl/codegen/kotlin/CliKotlinCodeGenerator.kt index 4c0681807..37052ee31 100644 --- a/pkl-codegen-kotlin/src/main/kotlin/org/pkl/codegen/kotlin/CliKotlinCodeGenerator.kt +++ b/pkl-codegen-kotlin/src/main/kotlin/org/pkl/codegen/kotlin/CliKotlinCodeGenerator.kt @@ -21,7 +21,7 @@ import org.pkl.commons.cli.CliException import org.pkl.commons.createParentDirectories import org.pkl.commons.writeString import org.pkl.core.ModuleSource -import org.pkl.core.util.Readers +import org.pkl.core.Readers /** API for the Kotlin code generator CLI. */ class CliKotlinCodeGenerator(private val options: CliKotlinCodeGeneratorOptions) : diff --git a/pkl-core/src/main/java/org/pkl/core/util/Readers.java b/pkl-core/src/main/java/org/pkl/core/Readers.java similarity index 97% rename from pkl-core/src/main/java/org/pkl/core/util/Readers.java rename to pkl-core/src/main/java/org/pkl/core/Readers.java index 3e4b140f3..1904c2da8 100644 --- a/pkl-core/src/main/java/org/pkl/core/util/Readers.java +++ b/pkl-core/src/main/java/org/pkl/core/Readers.java @@ -13,7 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.pkl.core.util; +package org.pkl.core; public final class Readers { private Readers() {} diff --git a/pkl-core/src/main/java/org/pkl/core/externalreader/ExternalReaderProcess.java b/pkl-core/src/main/java/org/pkl/core/externalreader/ExternalReaderProcess.java index 08d0bebbf..dc9a2d4dd 100644 --- a/pkl-core/src/main/java/org/pkl/core/externalreader/ExternalReaderProcess.java +++ b/pkl-core/src/main/java/org/pkl/core/externalreader/ExternalReaderProcess.java @@ -17,9 +17,10 @@ import java.io.IOException; import org.pkl.core.evaluatorSettings.PklEvaluatorSettings.ExternalReader; -import org.pkl.core.messaging.MessageTransport; import org.pkl.core.messaging.Messages.ModuleReaderSpec; import org.pkl.core.messaging.Messages.ResourceReaderSpec; +import org.pkl.core.module.ExternalModuleResolver; +import org.pkl.core.resource.ExternalResourceResolver; import org.pkl.core.util.Nullable; /** An external process that reads Pkl modules and resources. */ @@ -33,13 +34,23 @@ static ExternalReaderProcess of(ExternalReader spec) { } /** - * Returns a message transport for communicating with this process. + * Returns a resolver for modules provided via this reader. * *

Upon first call, this method may allocate resources, including spawning a child process. * * @throws IllegalStateException if this process has already been closed */ - MessageTransport getTransport() throws ExternalReaderProcessException; + ExternalModuleResolver getModuleResolver(long evaluatorId) throws ExternalReaderProcessException; + + /** + * Returns a resolver for resources provided via this reader. + * + *

Upon first call, this method may allocate resources, including spawning a child process. + * + * @throws IllegalStateException if this process has already been closed + */ + ExternalResourceResolver getResourceResolver(long evaluatorId) + throws ExternalReaderProcessException; /** * Returns the spec, if available, of this process's module reader with the given scheme. 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 3f4c35d75..833092090 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 @@ -35,6 +35,9 @@ import org.pkl.core.messaging.Messages.ModuleReaderSpec; import org.pkl.core.messaging.Messages.ResourceReaderSpec; import org.pkl.core.messaging.ProtocolException; +import org.pkl.core.module.ExternalModuleResolver; +import org.pkl.core.resource.ExternalResourceResolver; +import org.pkl.core.util.ErrorMessages; import org.pkl.core.util.LateInit; import org.pkl.core.util.Nullable; @@ -48,6 +51,7 @@ final class ExternalReaderProcessImpl implements ExternalReaderProcess { new ConcurrentHashMap<>(); private final Map> initializeResourceReaderResponses = new ConcurrentHashMap<>(); + private final Random requestIdGenerator = new Random(); private final Object lock = new Object(); private @GuardedBy("lock") boolean closed = false; @@ -75,7 +79,18 @@ private void log(String msg) { } @Override - public MessageTransport getTransport() throws ExternalReaderProcessException { + public ExternalModuleResolver getModuleResolver(long evaluatorId) + throws ExternalReaderProcessException { + return new ExternalModuleResolver(getTransport(), evaluatorId); + } + + @Override + public ExternalResourceResolver getResourceResolver(long evaluatorId) + throws ExternalReaderProcessException { + return new ExternalResourceResolver(getTransport(), evaluatorId); + } + + private MessageTransport getTransport() throws ExternalReaderProcessException { synchronized (lock) { if (closed) { throw new IllegalStateException("External reader process has already been closed."); @@ -83,7 +98,7 @@ public MessageTransport getTransport() throws ExternalReaderProcessException { if (process != null) { if (!process.isAlive()) { throw new ExternalReaderProcessException( - "External reader process has already terminated."); + ErrorMessages.create("externalReaderAlreadyTerminated")); } return transport; @@ -182,7 +197,8 @@ public void run() { uriScheme, (scheme) -> { var future = new CompletableFuture<@Nullable ModuleReaderSpec>(); - var request = new InitializeModuleReaderRequest(new Random().nextLong(), scheme); + var request = + new InitializeModuleReaderRequest(requestIdGenerator.nextLong(), scheme); try { getTransport() .send( @@ -209,7 +225,8 @@ public void run() { uriScheme, (scheme) -> { var future = new CompletableFuture<@Nullable ResourceReaderSpec>(); - var request = new InitializeResourceReaderRequest(new Random().nextLong(), scheme); + var request = + new InitializeResourceReaderRequest(requestIdGenerator.nextLong(), scheme); try { getTransport() .send( diff --git a/pkl-core/src/main/java/org/pkl/core/module/ExternalModuleResolver.java b/pkl-core/src/main/java/org/pkl/core/module/ExternalModuleResolver.java index 5d2c854e6..6ed608ca9 100644 --- a/pkl-core/src/main/java/org/pkl/core/module/ExternalModuleResolver.java +++ b/pkl-core/src/main/java/org/pkl/core/module/ExternalModuleResolver.java @@ -39,6 +39,7 @@ public class ExternalModuleResolver { private final long evaluatorId; private final Map> readResponses = new ConcurrentHashMap<>(); private final Map>> listResponses = new ConcurrentHashMap<>(); + private final Random requestIdGenerator = new Random(); public ExternalModuleResolver(MessageTransport transport, long evaluatorId) { this.transport = transport; @@ -74,7 +75,7 @@ private String doReadModule(URI moduleUri) throws IOException { moduleUri, (uri) -> { var future = new CompletableFuture(); - var request = new ReadModuleRequest(new Random().nextLong(), evaluatorId, uri); + var request = new ReadModuleRequest(requestIdGenerator.nextLong(), evaluatorId, uri); try { transport.send( request, @@ -104,7 +105,7 @@ private List doListElements(URI baseUri) throws IOException { baseUri, (uri) -> { var future = new CompletableFuture>(); - var request = new ListModulesRequest(new Random().nextLong(), evaluatorId, uri); + var request = new ListModulesRequest(requestIdGenerator.nextLong(), evaluatorId, uri); try { transport.send( request, diff --git a/pkl-core/src/main/java/org/pkl/core/module/ModuleKeyFactories.java b/pkl-core/src/main/java/org/pkl/core/module/ModuleKeyFactories.java index e242377f6..471af8a91 100644 --- a/pkl-core/src/main/java/org/pkl/core/module/ModuleKeyFactories.java +++ b/pkl-core/src/main/java/org/pkl/core/module/ModuleKeyFactories.java @@ -27,6 +27,7 @@ import java.util.Optional; import java.util.ServiceLoader; import javax.annotation.concurrent.GuardedBy; +import org.pkl.core.Readers; import org.pkl.core.externalreader.ExternalReaderProcess; import org.pkl.core.externalreader.ExternalReaderProcessException; import org.pkl.core.util.ErrorMessages; @@ -78,7 +79,7 @@ public static ModuleKeyFactory classPath(ClassLoader classLoader) { } /** - * Returns a factory for external reader module keys + * Returns a factory for external reader module keys. * *

NOTE: {@code process} needs to be {@link ExternalReaderProcess#close closed} to avoid * resource leaks. @@ -87,6 +88,12 @@ public static ModuleKeyFactory externalProcess(String scheme, ExternalReaderProc return new ExternalProcess(scheme, process, 0); } + /** + * Returns a factory for external reader module keys. + * + *

NOTE: {@code process} needs to be {@link ExternalReaderProcess#close closed} to avoid + * resource leaks. + */ public static ModuleKeyFactory externalProcess( String scheme, ExternalReaderProcess process, long evaluatorId) { return new ExternalProcess(scheme, process, evaluatorId); @@ -95,7 +102,7 @@ public static ModuleKeyFactory externalProcess( /** * Closes the given factories, ignoring any exceptions. * - * @deprecated Replaced by {@link org.pkl.core.util.Readers#closeQuietly}. + * @deprecated Replaced by {@link Readers#closeQuietly}. */ @Deprecated(since = "0.27.0", forRemoval = true) public static void closeQuietly(Iterable factories) { @@ -272,7 +279,7 @@ private synchronized ExternalModuleResolver getResolver() return resolver; } - resolver = new ExternalModuleResolver(process.getTransport(), evaluatorId); + resolver = process.getModuleResolver(evaluatorId); return resolver; } diff --git a/pkl-core/src/main/java/org/pkl/core/resource/ExternalResourceResolver.java b/pkl-core/src/main/java/org/pkl/core/resource/ExternalResourceResolver.java index 9f7711194..a1d8f21e1 100644 --- a/pkl-core/src/main/java/org/pkl/core/resource/ExternalResourceResolver.java +++ b/pkl-core/src/main/java/org/pkl/core/resource/ExternalResourceResolver.java @@ -38,6 +38,7 @@ public class ExternalResourceResolver { private final long evaluatorId; private final Map> readResponses = new ConcurrentHashMap<>(); private final Map>> listResponses = new ConcurrentHashMap<>(); + private final Random requestIdGenerator = new Random(); public ExternalResourceResolver(MessageTransport transport, long evaluatorId) { this.transport = transport; @@ -72,7 +73,8 @@ public List doListElements(URI baseUri) throws IOException { baseUri, (uri) -> { var future = new CompletableFuture>(); - var request = new ListResourcesRequest(new Random().nextLong(), evaluatorId, uri); + var request = + new ListResourcesRequest(requestIdGenerator.nextLong(), evaluatorId, uri); try { transport.send( request, @@ -101,7 +103,8 @@ public byte[] doRead(URI baseUri) throws IOException { baseUri, (uri) -> { var future = new CompletableFuture(); - var request = new ReadResourceRequest(new Random().nextLong(), evaluatorId, uri); + var request = + new ReadResourceRequest(requestIdGenerator.nextLong(), evaluatorId, uri); try { transport.send( request, diff --git a/pkl-core/src/main/java/org/pkl/core/resource/ResourceReaders.java b/pkl-core/src/main/java/org/pkl/core/resource/ResourceReaders.java index ae8ca7eb1..2ec70b96a 100644 --- a/pkl-core/src/main/java/org/pkl/core/resource/ResourceReaders.java +++ b/pkl-core/src/main/java/org/pkl/core/resource/ResourceReaders.java @@ -140,16 +140,28 @@ public static List fromServiceProviders() { return FromServiceProviders.INSTANCE; } - public static ResourceReader externalProcess( - String scheme, ExternalReaderProcess externalReaderProcess) { - return new ExternalProcess(scheme, externalReaderProcess, 0); + /** + * Returns a reader for external reader resources. + * + *

NOTE: {@code process} needs to be {@link ExternalReaderProcess#close closed} to avoid + * resource leaks. + */ + public static ResourceReader externalProcess(String scheme, ExternalReaderProcess process) { + return new ExternalProcess(scheme, process, 0); } + /** + * Returns a reader for external reader resources. + * + *

NOTE: {@code process} needs to be {@link ExternalReaderProcess#close closed} to avoid + * resource leaks. + */ public static ResourceReader externalProcess( - String scheme, ExternalReaderProcess externalReaderProcess, long evaluatorId) { - return new ExternalProcess(scheme, externalReaderProcess, evaluatorId); + String scheme, ExternalReaderProcess process, long evaluatorId) { + return new ExternalProcess(scheme, process, evaluatorId); } + /** Returns a reader for external and client reader resources. */ public static ResourceReader externalResolver( ResourceReaderSpec spec, ExternalResourceResolver resolver) { return new ExternalResolver(spec, resolver); @@ -637,9 +649,7 @@ private ExternalResolver getUnderlyingReader() throw new ExternalReaderProcessException( ErrorMessages.create("externalReaderDoesNotSupportScheme", "resource", scheme)); } - underlying = - new ExternalResolver( - spec, new ExternalResourceResolver(process.getTransport(), evaluatorId)); + underlying = new ExternalResolver(spec, process.getResourceResolver(evaluatorId)); return underlying; } diff --git a/pkl-core/src/main/java/org/pkl/core/service/ExecutorSpiImpl.java b/pkl-core/src/main/java/org/pkl/core/service/ExecutorSpiImpl.java index 1bd914829..6b9a4ebe7 100644 --- a/pkl-core/src/main/java/org/pkl/core/service/ExecutorSpiImpl.java +++ b/pkl-core/src/main/java/org/pkl/core/service/ExecutorSpiImpl.java @@ -28,12 +28,12 @@ import java.util.regex.Pattern; import java.util.stream.Collectors; import org.pkl.core.*; +import org.pkl.core.Readers; import org.pkl.core.http.HttpClient; import org.pkl.core.module.ModuleKeyFactories; import org.pkl.core.module.ModulePathResolver; import org.pkl.core.project.Project; import org.pkl.core.resource.ResourceReaders; -import org.pkl.core.util.Readers; import org.pkl.executor.spi.v1.ExecutorSpi; import org.pkl.executor.spi.v1.ExecutorSpiException; import org.pkl.executor.spi.v1.ExecutorSpiOptions; diff --git a/pkl-core/src/main/resources/org/pkl/core/errorMessages.properties b/pkl-core/src/main/resources/org/pkl/core/errorMessages.properties index 06772f9c4..9fa7dbf34 100644 --- a/pkl-core/src/main/resources/org/pkl/core/errorMessages.properties +++ b/pkl-core/src/main/resources/org/pkl/core/errorMessages.properties @@ -1117,3 +1117,6 @@ Failed to communicate with external reader process. externalReaderDoesNotSupportScheme=\ External {0} reader does not support scheme `{1}`. + +externalReaderAlreadyTerminated=\ +External reader process has already terminated. diff --git a/pkl-core/src/test/kotlin/org/pkl/core/externalreader/TestExternalReaderProcess.kt b/pkl-core/src/test/kotlin/org/pkl/core/externalreader/TestExternalReaderProcess.kt index 5e19cf9c0..5e22b0967 100644 --- a/pkl-core/src/test/kotlin/org/pkl/core/externalreader/TestExternalReaderProcess.kt +++ b/pkl-core/src/test/kotlin/org/pkl/core/externalreader/TestExternalReaderProcess.kt @@ -28,6 +28,8 @@ import org.pkl.core.messaging.MessageTransport import org.pkl.core.messaging.MessageTransports import org.pkl.core.messaging.Messages.* import org.pkl.core.messaging.ProtocolException +import org.pkl.core.module.ExternalModuleResolver +import org.pkl.core.resource.ExternalResourceResolver class TestExternalReaderProcess(private val transport: MessageTransport) : ExternalReaderProcess { private val initializeModuleReaderResponses: MutableMap> = @@ -40,7 +42,11 @@ class TestExternalReaderProcess(private val transport: MessageTransport) : Exter transport.close() } - override fun getTransport(): MessageTransport = transport + override fun getModuleResolver(evaluatorId: Long): ExternalModuleResolver = + ExternalModuleResolver(transport, evaluatorId) + + override fun getResourceResolver(evaluatorId: Long): ExternalResourceResolver = + ExternalResourceResolver(transport, evaluatorId) fun run() { try { diff --git a/pkl-doc/src/main/kotlin/org/pkl/doc/CliDocGenerator.kt b/pkl-doc/src/main/kotlin/org/pkl/doc/CliDocGenerator.kt index 499c6ba25..01341227c 100644 --- a/pkl-doc/src/main/kotlin/org/pkl/doc/CliDocGenerator.kt +++ b/pkl-doc/src/main/kotlin/org/pkl/doc/CliDocGenerator.kt @@ -24,8 +24,8 @@ import org.pkl.commons.cli.CliCommand import org.pkl.commons.cli.CliException import org.pkl.commons.toPath import org.pkl.core.* +import org.pkl.core.Readers import org.pkl.core.packages.* -import org.pkl.core.util.Readers /** * Entry point for the high-level Pkldoc API.