gnodet-bot commented on code in PR #11102: URL: https://github.com/apache/maven/pull/11102#discussion_r4252525054
########## impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/ProcessRuns.java: ########## @@ -0,0 +1,227 @@ +/* + * 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.cling.invoker; + +import java.io.IOException; +import java.io.Reader; +import java.io.Writer; +import java.nio.charset.StandardCharsets; +import java.nio.file.DirectoryStream; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.time.Duration; +import java.time.Instant; +import java.util.ArrayList; +import java.util.Comparator; +import java.util.List; +import java.util.Optional; +import java.util.Properties; + +/** + * Registry of the running Maven processes of the current user, kept as one descriptor file per process. + * All the operations are best effort: a registry which cannot be read or written never fails a build. + */ +public final class ProcessRuns { + + private static final String RUNS_SUBDIR = ".m2/.maven/runs"; + private static final String FILE_PREFIX = "mvn-"; + private static final String FILE_SUFFIX = ".properties"; + + private static final String KEY_PID = "pid"; + private static final String KEY_VERSION = "version"; + private static final String KEY_WORKDIR = "workDir"; + private static final String KEY_EXECROOT = "execRoot"; + private static final String KEY_STARTED = "started"; + + /** The start time of a process is not reported with the same precision all the time. */ + private static final Duration START_TOLERANCE = Duration.ofSeconds(2); + + private ProcessRuns() {} + + private static Path defaultRunsDir() { + return Paths.get(System.getProperty("user.home", ".")).resolve(RUNS_SUBDIR); + } + + private static Path descriptor(Path runsDir, long pid) { + return runsDir.resolve(FILE_PREFIX + pid + FILE_SUFFIX); + } + + /** + * Registers the current process. + */ + public static void install(String version, Path workDir, Path execRoot) { + ProcessHandle current = ProcessHandle.current(); + install( + defaultRunsDir(), + current.pid(), + current.info().startInstant().orElseGet(Instant::now), + version, + workDir, + execRoot); + } + + /** + * Removes the registration of the process. + */ + public static void uninstall(long pid) { + uninstall(defaultRunsDir(), pid); + } + + /** + * Describes the registered processes which are still running. + * + * @return a table of the processes, one line per process, without any trailing line separator + */ + public static String listAlive() { + return format(listAlive(defaultRunsDir())); + } + + static void install(Path runsDir, long pid, Instant started, String version, Path workDir, Path execRoot) { + try { + Files.createDirectories(runsDir); + Properties properties = new Properties(); + properties.setProperty(KEY_PID, Long.toString(pid)); + properties.setProperty(KEY_VERSION, version == null ? "-" : version); + properties.setProperty( + KEY_WORKDIR, + workDir == null ? "?" : workDir.toAbsolutePath().toString()); + properties.setProperty( + KEY_EXECROOT, + execRoot == null ? "?" : execRoot.toAbsolutePath().toString()); + properties.setProperty(KEY_STARTED, started.toString()); + try (Writer writer = Files.newBufferedWriter(descriptor(runsDir, pid), StandardCharsets.UTF_8)) { Review Comment: 💡 **Observation (non-blocking):** `install()` writes directly to the final descriptor path via `Files.newBufferedWriter()`. A concurrent `listAlive()` could theoretically read a partially-written file. A write-to-temp-then-atomic-move pattern would eliminate the race entirely: ```suggestion Path target = descriptor(runsDir, pid); Path tmp = runsDir.resolve(FILE_PREFIX + pid + ".tmp"); try (Writer writer = Files.newBufferedWriter(tmp, StandardCharsets.UTF_8)) { properties.store(writer, "mvn run"); } Files.move(tmp, target, java.nio.file.StandardCopyOption.ATOMIC_MOVE); ``` That said, since the whole registry is best-effort and `read()` already handles parse failures gracefully (`catch (IOException | RuntimeException e) → Optional.empty()`), the current approach is arguably acceptable. ########## impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/ProcessRuns.java: ########## @@ -0,0 +1,227 @@ +/* + * 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.cling.invoker; + +import java.io.IOException; +import java.io.Reader; +import java.io.Writer; +import java.nio.charset.StandardCharsets; +import java.nio.file.DirectoryStream; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.time.Duration; +import java.time.Instant; +import java.util.ArrayList; +import java.util.Comparator; +import java.util.List; +import java.util.Optional; +import java.util.Properties; + +/** + * Registry of the running Maven processes of the current user, kept as one descriptor file per process. + * All the operations are best effort: a registry which cannot be read or written never fails a build. + */ +public final class ProcessRuns { + + private static final String RUNS_SUBDIR = ".m2/.maven/runs"; + private static final String FILE_PREFIX = "mvn-"; + private static final String FILE_SUFFIX = ".properties"; + + private static final String KEY_PID = "pid"; + private static final String KEY_VERSION = "version"; + private static final String KEY_WORKDIR = "workDir"; + private static final String KEY_EXECROOT = "execRoot"; + private static final String KEY_STARTED = "started"; + + /** The start time of a process is not reported with the same precision all the time. */ + private static final Duration START_TOLERANCE = Duration.ofSeconds(2); + + private ProcessRuns() {} + + private static Path defaultRunsDir() { + return Paths.get(System.getProperty("user.home", ".")).resolve(RUNS_SUBDIR); + } + + private static Path descriptor(Path runsDir, long pid) { + return runsDir.resolve(FILE_PREFIX + pid + FILE_SUFFIX); + } + + /** + * Registers the current process. + */ + public static void install(String version, Path workDir, Path execRoot) { + ProcessHandle current = ProcessHandle.current(); + install( + defaultRunsDir(), + current.pid(), + current.info().startInstant().orElseGet(Instant::now), + version, + workDir, + execRoot); + } + + /** + * Removes the registration of the process. + */ + public static void uninstall(long pid) { + uninstall(defaultRunsDir(), pid); + } + + /** + * Describes the registered processes which are still running. + * + * @return a table of the processes, one line per process, without any trailing line separator + */ + public static String listAlive() { + return format(listAlive(defaultRunsDir())); + } + + static void install(Path runsDir, long pid, Instant started, String version, Path workDir, Path execRoot) { + try { + Files.createDirectories(runsDir); + Properties properties = new Properties(); + properties.setProperty(KEY_PID, Long.toString(pid)); + properties.setProperty(KEY_VERSION, version == null ? "-" : version); + properties.setProperty( + KEY_WORKDIR, + workDir == null ? "?" : workDir.toAbsolutePath().toString()); + properties.setProperty( + KEY_EXECROOT, + execRoot == null ? "?" : execRoot.toAbsolutePath().toString()); + properties.setProperty(KEY_STARTED, started.toString()); + try (Writer writer = Files.newBufferedWriter(descriptor(runsDir, pid), StandardCharsets.UTF_8)) { + properties.store(writer, "mvn run"); + } + } catch (IOException | RuntimeException ignored) { + // best effort + } + } + + static void uninstall(Path runsDir, long pid) { + try { + Files.deleteIfExists(descriptor(runsDir, pid)); + } catch (IOException ignored) { + // best effort + } + } + + static List<Run> listAlive(Path runsDir) { + List<Run> runs = new ArrayList<>(); + if (!Files.isDirectory(runsDir)) { + return runs; + } + try (DirectoryStream<Path> descriptors = Files.newDirectoryStream(runsDir, FILE_PREFIX + "*" + FILE_SUFFIX)) { + for (Path file : descriptors) { + Optional<Run> run = read(file); + if (run.isPresent() && isRunning(run.get())) { + runs.add(run.get()); + } else { + // stale: the process is gone, or its pid was taken by another process since + Files.deleteIfExists(file); + } + } + } catch (IOException ignored) { + // return what was gathered + } + runs.sort(Comparator.comparing((Run run) -> run.started).thenComparingLong(run -> run.pid)); + return runs; + } + + private static Optional<Run> read(Path file) { + Properties properties = new Properties(); + try (Reader reader = Files.newBufferedReader(file, StandardCharsets.UTF_8)) { + properties.load(reader); + return Optional.of(new Run( + Long.parseLong(properties.getProperty(KEY_PID)), + properties.getProperty(KEY_VERSION, "-"), + properties.getProperty(KEY_WORKDIR, "?"), + properties.getProperty(KEY_EXECROOT, "?"), + properties.getProperty(KEY_STARTED, "?"))); + } catch (IOException | RuntimeException e) { + return Optional.empty(); + } + } + + /** + * Tells whether the process of the descriptor is running. A pid alone is not enough, as the system hands it over + * to another process once the first one is gone, so the start time recorded has to be the one of the process too. + */ + private static boolean isRunning(Run run) { + Optional<ProcessHandle> process = ProcessHandle.of(run.pid).filter(ProcessHandle::isAlive); + if (process.isEmpty()) { + return false; + } + Optional<Instant> actualStart = process.get().info().startInstant(); + if (actualStart.isEmpty()) { + // the system does not tell, nothing to compare with + return true; + } + try { + return Duration.between(Instant.parse(run.started), actualStart.get()) + .abs() + .compareTo(START_TOLERANCE) + <= 0; + } catch (RuntimeException e) { + return false; + } + } + + static String format(List<Run> runs) { + if (runs.isEmpty()) { + return "No running Maven processes."; + } + StringBuilder sb = new StringBuilder(); + sb.append(String.format("%-10s %-12s %-24s %-48s %-48s", "PID", "VERSION", "STARTED", "WORKDIR", "EXEC_ROOT")); Review Comment: 💡 **Observation (non-blocking):** Column widths are hardcoded to `%-48s` for `WORKDIR` and `EXEC_ROOT`. On narrow terminals (or with long paths that get truncated to 48 chars by `truncateStart()`), the output will wrap. Consider computing the column width dynamically from the actual max values in the run list, or at least capping the output to the terminal width. The `truncateStart()` helper already handles the path side — the column width itself could follow suit. -- 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]
