Skip to content

Commit 8d78d28

Browse files
odenixodenix
committed
Implement v2 of executor SPI
This allows to support certificate files without introducing breaking changes. - undo breaking changes to spi.v1 - play it safe and keep spi.v2 totally independent of spi.v1 (no `ExecutorSpiOptions2 extends ExecutorSpiOptions` etc.) - run executor tests against both SPI versions - switch to `getPlatformClassLoader()` as indicated in "once we move to JDK9+" comments
1 parent 09be2f4 commit 8d78d28

File tree

9 files changed

+449
-181
lines changed

9 files changed

+449
-181
lines changed

‎pkl-core/src/main/java/org/pkl/core/service/ExecutorSpiImpl.java‎

Lines changed: 95 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import java.nio.file.Path;
2222
import java.util.Collections;
2323
import java.util.LinkedHashMap;
24+
import java.util.List;
2425
import java.util.Map;
2526
import java.util.Map.Entry;
2627
import java.util.Objects;
@@ -36,8 +37,11 @@
3637
import org.pkl.executor.spi.v1.ExecutorSpi;
3738
import org.pkl.executor.spi.v1.ExecutorSpiException;
3839
import org.pkl.executor.spi.v1.ExecutorSpiOptions;
40+
import org.pkl.executor.spi.v2.ExecutorSpi2;
41+
import org.pkl.executor.spi.v2.ExecutorSpiException2;
42+
import org.pkl.executor.spi.v2.ExecutorSpiOptions2;
3943

40-
public class ExecutorSpiImpl implements ExecutorSpi {
44+
public class ExecutorSpiImpl implements ExecutorSpi, ExecutorSpi2 {
4145
private static final int MAX_HTTP_CLIENTS = 3;
4246

4347
// Don't create a new HTTP client for every executor request.
@@ -66,33 +70,94 @@ public String getPklVersion() {
6670

6771
@Override
6872
public String evaluatePath(Path modulePath, ExecutorSpiOptions options) {
69-
var allowedModules =
70-
options.getAllowedModules().stream().map(Pattern::compile).collect(Collectors.toList());
73+
var builder =
74+
createEvaluatorBuilder(
75+
options.getAllowedModules(),
76+
options.getAllowedResources(),
77+
options.getRootDir(),
78+
options.getModulePath(),
79+
options.getEnvironmentVariables(),
80+
options.getExternalProperties(),
81+
options.getTimeout(),
82+
options.getOutputFormat(),
83+
options.getModuleCacheDir(),
84+
options.getProjectDir(),
85+
List.of(),
86+
List.of());
7187

72-
var allowedResources =
73-
options.getAllowedResources().stream().map(Pattern::compile).collect(Collectors.toList());
88+
try (var evaluator = builder.build()) {
89+
return evaluator.evaluateOutputText(ModuleSource.path(modulePath));
90+
} catch (PklException e) {
91+
throw new ExecutorSpiException(e.getMessage(), e.getCause());
92+
} finally {
93+
ModuleKeyFactories.closeQuietly(builder.getModuleKeyFactories());
94+
}
95+
}
96+
97+
@Override
98+
public String evaluatePath(Path modulePath, ExecutorSpiOptions2 options) {
99+
var builder =
100+
createEvaluatorBuilder(
101+
options.getAllowedModules(),
102+
options.getAllowedResources(),
103+
options.getRootDir(),
104+
options.getModulePath(),
105+
options.getEnvironmentVariables(),
106+
options.getExternalProperties(),
107+
options.getTimeout(),
108+
options.getOutputFormat(),
109+
options.getModuleCacheDir(),
110+
options.getProjectDir(),
111+
options.getCertificateFiles(),
112+
options.getCertificateUris());
113+
114+
try (var evaluator = builder.build()) {
115+
return evaluator.evaluateOutputText(ModuleSource.path(modulePath));
116+
} catch (PklException e) {
117+
throw new ExecutorSpiException2(e.getMessage(), e.getCause());
118+
} finally {
119+
ModuleKeyFactories.closeQuietly(builder.getModuleKeyFactories());
120+
}
121+
}
122+
123+
private EvaluatorBuilder createEvaluatorBuilder(
124+
List<String> allowedModules,
125+
List<String> allowedResources,
126+
Path rootDir,
127+
List<Path> modulePath,
128+
Map<String, String> environmentVariables,
129+
Map<String, String> externalProperties,
130+
java.time.Duration timeout,
131+
String outputFormat,
132+
Path moduleCacheDir,
133+
Path projectDir,
134+
List<Path> certificateFiles,
135+
List<URI> certificateUris) {
136+
var allowedModulePatterns =
137+
allowedModules.stream().map(Pattern::compile).collect(Collectors.toList());
138+
139+
var allowedResourcePatterns =
140+
allowedResources.stream().map(Pattern::compile).collect(Collectors.toList());
74141

75142
var securityManager =
76143
SecurityManagers.standard(
77-
allowedModules,
78-
allowedResources,
144+
allowedModulePatterns,
145+
allowedResourcePatterns,
79146
SecurityManagers.defaultTrustLevels,
80-
options.getRootDir());
147+
rootDir);
81148

82149
var transformer = StackFrameTransformers.defaultTransformer;
83-
if (options.getRootDir() != null) {
150+
if (rootDir != null) {
84151
transformer =
85-
transformer.andThen(
86-
StackFrameTransformers.relativizeModuleUri(options.getRootDir().toUri()));
152+
transformer.andThen(StackFrameTransformers.relativizeModuleUri(rootDir.toUri()));
87153
}
88154

89-
var resolver = new ModulePathResolver(options.getModulePath());
90-
155+
var resolver = new ModulePathResolver(modulePath);
91156
var builder =
92157
EvaluatorBuilder.unconfigured()
93158
.setStackFrameTransformer(transformer)
94159
.setSecurityManager(securityManager)
95-
.setHttpClient(getOrCreateHttpClient(options))
160+
.setHttpClient(getOrCreateHttpClient(certificateFiles, certificateUris))
96161
.addResourceReader(ResourceReaders.environmentVariable())
97162
.addResourceReader(ResourceReaders.externalProperty())
98163
.addResourceReader(ResourceReaders.modulePath(resolver))
@@ -108,27 +173,22 @@ public String evaluatePath(Path modulePath, ExecutorSpiOptions options) {
108173
.addModuleKeyFactory(ModuleKeyFactories.projectpackage)
109174
.addModuleKeyFactory(ModuleKeyFactories.file)
110175
.addModuleKeyFactory(ModuleKeyFactories.genericUrl)
111-
.setEnvironmentVariables(options.getEnvironmentVariables())
112-
.setExternalProperties(options.getExternalProperties())
113-
.setTimeout(options.getTimeout())
114-
.setOutputFormat(options.getOutputFormat())
115-
.setModuleCacheDir(options.getModuleCacheDir());
116-
if (options.getProjectDir() != null) {
117-
var project = Project.loadFromPath(options.getProjectDir().resolve(PKL_PROJECT_FILENAME));
176+
.setEnvironmentVariables(environmentVariables)
177+
.setExternalProperties(externalProperties)
178+
.setTimeout(timeout)
179+
.setOutputFormat(outputFormat)
180+
.setModuleCacheDir(moduleCacheDir);
181+
182+
if (projectDir != null) {
183+
var project = Project.loadFromPath(projectDir.resolve(PKL_PROJECT_FILENAME));
118184
builder.setProjectDependencies(project.getDependencies());
119185
}
120186

121-
try (var evaluator = builder.build()) {
122-
return evaluator.evaluateOutputText(ModuleSource.path(modulePath));
123-
} catch (PklException e) {
124-
throw new ExecutorSpiException(e.getMessage(), e.getCause());
125-
} finally {
126-
ModuleKeyFactories.closeQuietly(builder.getModuleKeyFactories());
127-
}
187+
return builder;
128188
}
129189

130-
private HttpClient getOrCreateHttpClient(ExecutorSpiOptions options) {
131-
var clientKey = new HttpClientKey(options);
190+
private HttpClient getOrCreateHttpClient(List<Path> certificateFiles, List<URI> certificateUris) {
191+
var clientKey = new HttpClientKey(certificateFiles, certificateUris);
132192
return httpClients.computeIfAbsent(
133193
clientKey,
134194
(key) -> {
@@ -140,7 +200,7 @@ private HttpClient getOrCreateHttpClient(ExecutorSpiOptions options) {
140200
builder.addCertificates(uri);
141201
}
142202
// If the above didn't add any certificates,
143-
// builder will fall back to Pkl's built-in certificates.
203+
// builder will use the JVM's default SSL context.
144204
return builder.build();
145205
});
146206
}
@@ -149,10 +209,10 @@ private static final class HttpClientKey {
149209
final Set<Path> certificateFiles;
150210
final Set<URI> certificateUris;
151211

152-
HttpClientKey(ExecutorSpiOptions options) {
153-
// make defensive copies
154-
certificateFiles = Set.copyOf(options.getCertificateFiles());
155-
certificateUris = Set.copyOf(options.getCertificateUris());
212+
HttpClientKey(List<Path> certificateFiles, List<URI> certificateUris) {
213+
// also serve as defensive copies
214+
this.certificateFiles = Set.copyOf(certificateFiles);
215+
this.certificateUris = Set.copyOf(certificateUris);
156216
}
157217

158218
@Override

‎pkl-core/src/main/resources/META-INF/services/org.pkl.executor.spi.v2.ExecutorSpi2‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
org.pkl.core.service.ExecutorSpiImpl

‎pkl-executor/src/main/java/org/pkl/executor/EmbeddedExecutor.java‎

Lines changed: 71 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,9 @@
2828
import org.pkl.executor.spi.v1.ExecutorSpi;
2929
import org.pkl.executor.spi.v1.ExecutorSpiException;
3030
import org.pkl.executor.spi.v1.ExecutorSpiOptions;
31+
import org.pkl.executor.spi.v2.ExecutorSpi2;
32+
import org.pkl.executor.spi.v2.ExecutorSpiException2;
33+
import org.pkl.executor.spi.v2.ExecutorSpiOptions2;
3134
import org.slf4j.Logger;
3235
import org.slf4j.LoggerFactory;
3336

@@ -40,12 +43,17 @@ final class EmbeddedExecutor implements Executor {
4043
private final List<PklDistribution> pklDistributions = new ArrayList<>();
4144

4245
/**
43-
* @throws IllegalArgumentException if a Jar file cannot be found or is not a valid PklPkl
46+
* @throws IllegalArgumentException if a Jar file cannot be found or is not a valid Pkl
4447
* distribution
4548
*/
4649
public EmbeddedExecutor(List<Path> pklFatJars) {
50+
this(pklFatJars, 0);
51+
}
52+
53+
// for testing only
54+
EmbeddedExecutor(List<Path> pklFatJars, int spiVersion) {
4755
for (var jarFile : pklFatJars) {
48-
pklDistributions.add(new PklDistribution(jarFile));
56+
pklDistributions.add(new PklDistribution(jarFile, spiVersion));
4957
}
5058
}
5159

@@ -163,41 +171,61 @@ public void close() throws Exception {
163171
}
164172

165173
private static final class PklDistribution implements AutoCloseable {
166-
final URLClassLoader classLoader;
167-
final ExecutorSpi executorSpi;
174+
final URLClassLoader pklDistributionClassLoader;
175+
final /* @Nullable */ ExecutorSpi executorSpi;
176+
final /* @Nullable */ ExecutorSpi2 executorSpi2;
168177
final Version version;
169178

170179
/**
171180
* @throws IllegalArgumentException if the Jar file does not exist or is not a valid Pkl
172181
* distribution
173182
*/
174-
PklDistribution(Path pklFatJar) {
183+
PklDistribution(Path pklFatJar, /* 0 -> choose highest available */ int spiVersion) {
175184
if (!Files.isRegularFile(pklFatJar)) {
176185
throw new IllegalArgumentException(
177186
String.format("Invalid Pkl distribution: Cannot find Jar file `%s`.", pklFatJar));
178187
}
179188

180-
classLoader = new PklDistributionClassLoader(pklFatJar);
181-
var serviceLoader = ServiceLoader.load(ExecutorSpi.class, classLoader);
189+
pklDistributionClassLoader = new PklDistributionClassLoader(pklFatJar);
182190

183191
try {
184-
executorSpi = serviceLoader.iterator().next();
185-
} catch (NoSuchElementException e) {
186-
throw new IllegalArgumentException(
187-
String.format(
188-
"Invalid Pkl distribution: Cannot find service of type `%s` in Jar file `%s`.",
189-
ExecutorSpi.class.getTypeName(), pklFatJar));
190-
192+
if (spiVersion == 0 || spiVersion == 2) {
193+
var serviceLoader2 = ServiceLoader.load(ExecutorSpi2.class, pklDistributionClassLoader);
194+
executorSpi2 = serviceLoader2.findFirst().orElse(null);
195+
if (executorSpi2 != null) {
196+
executorSpi = null;
197+
// convert to normal to allow running with a dev version
198+
version = Version.parse(executorSpi2.getPklVersion()).toNormal();
199+
return;
200+
}
201+
} else {
202+
executorSpi2 = null;
203+
}
204+
if (spiVersion == 0 || spiVersion == 1) {
205+
var serviceLoader = ServiceLoader.load(ExecutorSpi.class, pklDistributionClassLoader);
206+
executorSpi = serviceLoader.findFirst().orElse(null);
207+
if (executorSpi != null) {
208+
// convert to normal to allow running with a dev version
209+
version = Version.parse(executorSpi.getPklVersion()).toNormal();
210+
return;
211+
}
212+
} else {
213+
executorSpi = null;
214+
}
215+
if (spiVersion == 0) {
216+
throw new IllegalArgumentException(
217+
String.format(
218+
"Invalid Pkl distribution: Cannot find service of type `%s` in Jar file `%s`.",
219+
ExecutorSpi.class.getTypeName(), pklFatJar));
220+
}
221+
throw new IllegalArgumentException("Invalid SPI version: " + spiVersion);
191222
} catch (ServiceConfigurationError e) {
192223
throw new IllegalArgumentException(
193224
String.format(
194225
"Invalid Pkl distribution: Unexpected error loading service of type `%s` from Jar file `%s`.",
195226
ExecutorSpi.class.getTypeName(), pklFatJar),
196227
e);
197228
}
198-
199-
// convert to normal to allow running with a dev version
200-
version = Version.parse(executorSpi.getPklVersion()).toNormal();
201229
}
202230

203231
Version getVersion() {
@@ -208,10 +236,12 @@ String evaluatePath(Path modulePath, ExecutorOptions options) {
208236
var currentThread = Thread.currentThread();
209237
var prevContextClassLoader = currentThread.getContextClassLoader();
210238
// Truffle loads stuff from context class loader, so set it to our class loader
211-
currentThread.setContextClassLoader(classLoader);
239+
currentThread.setContextClassLoader(pklDistributionClassLoader);
212240
try {
213-
return executorSpi.evaluatePath(modulePath, toEvaluatorOptions(options));
214-
} catch (ExecutorSpiException e) {
241+
return executorSpi2 != null
242+
? executorSpi2.evaluatePath(modulePath, toSpiOptions2(options))
243+
: executorSpi.evaluatePath(modulePath, toSpiOptions(options));
244+
} catch (ExecutorSpiException | ExecutorSpiException2 e) {
215245
throw new ExecutorException(e.getMessage(), e.getCause());
216246
} finally {
217247
currentThread.setContextClassLoader(prevContextClassLoader);
@@ -220,24 +250,38 @@ String evaluatePath(Path modulePath, ExecutorOptions options) {
220250

221251
@Override
222252
public void close() throws IOException {
223-
classLoader.close();
253+
pklDistributionClassLoader.close();
224254
}
225255

226-
ExecutorSpiOptions toEvaluatorOptions(ExecutorOptions options) {
256+
ExecutorSpiOptions toSpiOptions(ExecutorOptions options) {
227257
return new ExecutorSpiOptions(
228258
options.getAllowedModules(),
229259
options.getAllowedResources(),
230260
options.getEnvironmentVariables(),
231261
options.getExternalProperties(),
232262
options.getModulePath(),
233-
options.getCertificateFiles(),
234-
options.getCertificateUris(),
235263
options.getRootDir(),
236264
options.getTimeout(),
237265
options.getOutputFormat(),
238266
options.getModuleCacheDir(),
239267
options.getProjectDir());
240268
}
269+
270+
ExecutorSpiOptions2 toSpiOptions2(ExecutorOptions options) {
271+
return new ExecutorSpiOptions2(
272+
options.getAllowedModules(),
273+
options.getAllowedResources(),
274+
options.getModulePath(),
275+
options.getRootDir(),
276+
options.getTimeout(),
277+
options.getOutputFormat(),
278+
options.getModuleCacheDir(),
279+
options.getProjectDir(),
280+
options.getEnvironmentVariables(),
281+
options.getExternalProperties(),
282+
options.getCertificateFiles(),
283+
options.getCertificateUris());
284+
}
241285
}
242286

243287
private static final class PklDistributionClassLoader extends URLClassLoader {
@@ -284,18 +328,14 @@ protected Class<?> loadClass(String name, boolean resolve) throws ClassNotFoundE
284328

285329
@Override
286330
public URL getResource(String name) {
287-
// try bootstrap class loader first
288-
// once we move to JDK 9+, should use `getPlatformClassLoader().getResource()` instead of
289-
// `super.getResource()`
290-
var resource = super.getResource(name);
331+
var resource = getPlatformClassLoader().getResource(name);
291332
return resource != null ? resource : findResource(name);
292333
}
293334

294335
@Override
295336
public Enumeration<URL> getResources(String name) throws IOException {
296-
// once we move to JDK 9+, should use `getPlatformClassLoader().getResources()` instead of
297-
// `super.getResources()`
298-
return ConcatenatedEnumeration.create(super.getResources(name), findResources(name));
337+
return ConcatenatedEnumeration.create(
338+
getPlatformClassLoader().getResources(name), findResources(name));
299339
}
300340

301341
static URL[] toUrls(Path pklFatJar) {

0 commit comments

Comments
 (0)