Copilot commented on code in PR #218:
URL:
https://github.com/apache/maven-script-interpreter/pull/218#discussion_r4132494993
##########
src/main/java/org/apache/maven/shared/scriptinterpreter/GroovyScriptInterpreter.java:
##########
@@ -140,33 +140,24 @@ static String normalizeTargetBytecode(String version) {
@Override
public Object evaluateScript(String script, Map<String, ?>
globalVariables, PrintStream scriptOutput)
throws ScriptEvaluationException {
- PrintStream origOut = System.out;
- PrintStream origErr = System.err;
-
ClassLoader curentClassLoader =
Thread.currentThread().getContextClassLoader();
- try {
-
- if (scriptOutput != null) {
- System.setErr(scriptOutput);
- System.setOut(scriptOutput);
- }
-
+ try (ScriptOutputRedirect redirect = scriptOutput != null ?
ScriptOutputRedirect.to(scriptOutput) : null) {
CompilerConfiguration compilerConfiguration = new
CompilerConfiguration(CompilerConfiguration.DEFAULT);
if (targetBytecode != null) {
compilerConfiguration.setTargetBytecode(normalizeTargetBytecode(targetBytecode));
}
-
- GroovyShell interpreter =
- new GroovyShell(childFirstLoader, new
Binding(globalVariables), compilerConfiguration);
-
+ Binding binding = new Binding(globalVariables);
+ if (scriptOutput != null) {
+ // println/print/printf in a Groovy script go to the "out"
variable when one is bound
+ binding.setVariable("out", scriptOutput);
+ }
Review Comment:
`Binding(Map)` keeps the supplied map, so `setVariable("out", ...)` mutates
`globalVariables`. A caller may legitimately pass an immutable map through the
public `ScriptInterpreter` API; any Groovy evaluation with an output stream
will now fail with `UnsupportedOperationException`, even when the script never
assigns a variable. Populate a fresh binding before adding `out`.
##########
src/main/java/org/apache/maven/shared/scriptinterpreter/ScriptOutputRedirect.java:
##########
@@ -0,0 +1,278 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.maven.shared.scriptinterpreter;
+
+import java.io.IOException;
+import java.io.OutputStream;
+import java.io.PrintStream;
+import java.util.Locale;
+
+/**
+ * Routes {@code System.out} and {@code System.err} of the current thread to a
script's log while that script runs,
+ * without taking the streams away from the rest of the JVM.
+ * <p>
+ * The process-wide streams are replaced once, while at least one redirect is
active, by streams that dispatch each
+ * write to the redirect registered for the calling thread (and threads it
spawns), or to the original stream when
+ * there is none. When the last redirect ends the original streams are put
back.
+ */
+final class ScriptOutputRedirect implements AutoCloseable {
+
+ private static final Object LOCK = new Object();
+
+ private static final InheritableThreadLocal<PrintStream> CURRENT = new
InheritableThreadLocal<>();
Review Comment:
The inheritable value is the raw target stream, and `close()` clears it only
from the evaluation thread. A child thread that outlives the script therefore
retains the old target indefinitely; while another script keeps or later
installs the dispatcher, that child's output is sent to the prior script's
closed log instead of the real stream. Inherit a redirect context that can be
marked closed, and resolve past closed contexts to an active parent or the
fallback.
##########
src/main/java/org/apache/maven/shared/scriptinterpreter/ScriptOutputRedirect.java:
##########
@@ -0,0 +1,278 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.maven.shared.scriptinterpreter;
+
+import java.io.IOException;
+import java.io.OutputStream;
+import java.io.PrintStream;
+import java.util.Locale;
+
+/**
+ * Routes {@code System.out} and {@code System.err} of the current thread to a
script's log while that script runs,
+ * without taking the streams away from the rest of the JVM.
+ * <p>
+ * The process-wide streams are replaced once, while at least one redirect is
active, by streams that dispatch each
+ * write to the redirect registered for the calling thread (and threads it
spawns), or to the original stream when
+ * there is none. When the last redirect ends the original streams are put
back.
+ */
+final class ScriptOutputRedirect implements AutoCloseable {
+
+ private static final Object LOCK = new Object();
+
+ private static final InheritableThreadLocal<PrintStream> CURRENT = new
InheritableThreadLocal<>();
+
+ private static int active;
+
+ private static PrintStream originalOut;
+
+ private static PrintStream originalErr;
+
+ private final PrintStream previous;
+
+ private ScriptOutputRedirect(PrintStream target) {
+ synchronized (LOCK) {
+ if (active++ == 0) {
+ originalOut = System.out;
+ originalErr = System.err;
+ System.setOut(new DispatchingPrintStream(originalOut));
+ System.setErr(new DispatchingPrintStream(originalErr));
+ }
+ }
+ previous = CURRENT.get();
+ CURRENT.set(target);
Review Comment:
If an overlapping caller passes the current `System.out` as `scriptOutput`,
that value is this dispatcher. Registering it as the current target makes the
next `System.out.print*` call dispatch back to itself until
`StackOverflowError`. Resolve an existing dispatcher to its effective target
before storing it.
##########
src/test/java/org/apache/maven/shared/scriptinterpreter/ScriptOutputRedirectTest.java:
##########
@@ -0,0 +1,118 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.maven.shared.scriptinterpreter;
+
+import java.io.ByteArrayOutputStream;
+import java.io.File;
+import java.io.PrintStream;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.concurrent.Callable;
+import java.util.concurrent.CyclicBarrier;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Script output goes to the script's own log, other threads keep the real
streams, and scripts do not wait for
+ * each other.
+ */
+class ScriptOutputRedirectTest {
+
+ @TempDir
+ File tempDir;
+
+ @Test
+ void concurrentScriptsKeepTheirOutputApart() throws Exception {
+ PrintStream originalOut = System.out;
+ PrintStream originalErr = System.err;
+ ByteArrayOutputStream outsideOut = new ByteArrayOutputStream();
+ System.setOut(new PrintStream(outsideOut, true, "UTF-8"));
+ ExecutorService pool = Executors.newFixedThreadPool(3);
+ try {
+ // the two scripts wait for each other at the barrier, so they are
running at the same time
+ CyclicBarrier barrier = new CyclicBarrier(2);
+ Future<String> first = pool.submit(script("first", barrier));
+ Future<String> second = pool.submit(script("second", barrier));
+ Future<?> bystander = pool.submit(() -> {
+ System.out.println("bystander line");
+ return null;
+ });
Review Comment:
The bystander is not synchronized with either script, so it may print before
the redirects are installed and let cross-thread capture pass undetected.
Coordinate it with the scripts and keep the script evaluations open until its
print completes, rather than relying on task scheduling and the 50 ms sleep.
##########
src/test/java/org/apache/maven/shared/scriptinterpreter/ScriptOutputRedirectTest.java:
##########
@@ -0,0 +1,118 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.maven.shared.scriptinterpreter;
+
+import java.io.ByteArrayOutputStream;
+import java.io.File;
+import java.io.PrintStream;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.concurrent.Callable;
+import java.util.concurrent.CyclicBarrier;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Script output goes to the script's own log, other threads keep the real
streams, and scripts do not wait for
+ * each other.
+ */
+class ScriptOutputRedirectTest {
+
+ @TempDir
+ File tempDir;
+
+ @Test
+ void concurrentScriptsKeepTheirOutputApart() throws Exception {
+ PrintStream originalOut = System.out;
+ PrintStream originalErr = System.err;
+ ByteArrayOutputStream outsideOut = new ByteArrayOutputStream();
+ System.setOut(new PrintStream(outsideOut, true, "UTF-8"));
+ ExecutorService pool = Executors.newFixedThreadPool(3);
+ try {
+ // the two scripts wait for each other at the barrier, so they are
running at the same time
+ CyclicBarrier barrier = new CyclicBarrier(2);
+ Future<String> first = pool.submit(script("first", barrier));
+ Future<String> second = pool.submit(script("second", barrier));
+ Future<?> bystander = pool.submit(() -> {
+ System.out.println("bystander line");
+ return null;
+ });
+ bystander.get();
+ String firstLog = first.get();
+ String secondLog = second.get();
+
+ assertTrue(firstLog.contains("start first") &&
firstLog.contains("system first"), firstLog);
+ assertTrue(firstLog.contains("error first") &&
firstLog.contains("end first"), firstLog);
+ assertFalse(firstLog.contains("second"), firstLog);
+ assertTrue(secondLog.contains("start second") &&
secondLog.contains("system second"), secondLog);
+ assertFalse(secondLog.contains("first"), secondLog);
Review Comment:
The test never checks that the second script's `System.err` and final Groovy
`println` reached its log, although those are part of the behavior under test.
A concurrency regression that drops either output can pass as long as the first
script is handled correctly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
