You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "arturobernalg (via GitHub)" <gi...@apache.org> on 2025/11/07 12:54:32 UTC
[PR] Introduce TempFileService and lifecycle cleanup participant for Maven 3. [maven]
arturobernalg opened a new pull request, #11405:
URL: https://github.com/apache/maven/pull/11405
Following this checklist to help us incorporate your
contribution quickly and easily:
[X ] Your pull request should address just one issue, without pulling in other changes.
[ X] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
[ X] Each commit in the pull request should have a meaningful subject line and body.
Note that commits might be squashed by a maintainer on merge.
[X ] Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied.
This may not always be possible but is a best-practice.
[X ] Run mvn verify to make sure basic checks pass.
A more thorough check will be performed on your pull request automatically.
You have run the [Core IT](https://maven.apache.org/core-its/core-it-suite/) successfully.
If your pull request is about ~20 lines of code you don't need to sign an
[Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf) if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introduce TempFileService and lifecycle cleanup participant [maven]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #11405:
URL: https://github.com/apache/maven/pull/11405#discussion_r4269196362
##########
impl/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultTempFileService.java:
##########
@@ -0,0 +1,184 @@
+/*
+ * 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.internal.impl;
+
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import java.io.IOException;
+import java.nio.file.FileVisitOption;
+import java.nio.file.FileVisitResult;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.SimpleFileVisitor;
+import java.nio.file.attribute.BasicFileAttributes;
+import java.util.Collections;
+import java.util.EnumSet;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Supplier;
+
+import org.apache.maven.api.Session;
+import org.apache.maven.api.SessionData;
+import org.apache.maven.api.services.TempFileService;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.apache.maven.api.Constants.MAVEN_TEMPFILE_KEEP;
+
+/**
+ * Default TempFileService implementation.
+ * Stores tracked paths in Session-scoped data and removes them after the build.
+ */
+@Named
+@Singleton
+public final class DefaultTempFileService implements TempFileService {
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(DefaultTempFileService.class);
+
+ // unique, typed session key (uses factory; one narrow unchecked cast)
+ @SuppressWarnings({"unchecked", "rawtypes"})
+ private static final SessionData.Key<Set<Path>> TMP_KEY =
+ (SessionData.Key) SessionData.key(Set.class, DefaultTempFileService.class);
+
+ // supplier with concrete types (avoids inference noise)
+ private static final Supplier<Set<Path>> TMP_SUPPLIER =
+ () -> Collections.newSetFromMap(new ConcurrentHashMap<Path, Boolean>());
+
+ @Override
+ public Path createTempFile(final Session session, final String prefix, final String suffix) throws IOException {
+ Objects.requireNonNull(session, "session");
+ final Path file = Files.createTempFile(prefix, suffix);
+ register(session, file);
+ return file;
+ }
+
+ @Override
+ public Path createTempFile(final Session session, final String prefix, final String suffix, final Path directory)
+ throws IOException {
+ Objects.requireNonNull(session, "session");
+ Objects.requireNonNull(directory, "directory");
+ final Path file = Files.createTempFile(directory, prefix, suffix);
+ register(session, file);
+ return file;
+ }
+
+ @Override
+ public Path createTempDirectory(final Session session, final String prefix) throws IOException {
+ Objects.requireNonNull(session, "session");
+ final Path dir = Files.createTempDirectory(prefix);
+ register(session, dir);
+ return dir;
+ }
+
+ @Override
+ public Path createTempDirectory(final Session session, final String prefix, final Path directory)
+ throws IOException {
+ Objects.requireNonNull(session, "session");
+ Objects.requireNonNull(directory, "directory");
+ final Path dir = Files.createTempDirectory(directory, prefix);
+ register(session, dir);
+ return dir;
+ }
+
+ @Override
+ public void register(final Session session, final Path path) {
+ Objects.requireNonNull(session, "session");
+ Objects.requireNonNull(path, "path");
+ final Set<Path> bucket = sessionPaths(session);
+ bucket.add(path);
+ if (LOGGER.isDebugEnabled()) {
+ LOGGER.debug("Temp path registered for cleanup: {}", path);
+ }
+ }
+
+ @Override
+ public void cleanup(final Session session) throws IOException {
+ Objects.requireNonNull(session, "session");
+
+ if (Boolean.getBoolean(MAVEN_TEMPFILE_KEEP)) {
Review Comment:
📝 **`isInfoEnabled()` guard is redundant**
SLF4J's format-string overload (`LOGGER.info("... {}", value)`) is already lazy — it only formats the message if INFO is enabled. The explicit `isInfoEnabled()` check adds noise without benefit. Same applies to the `isDebugEnabled()` guard in `register()` above.
```suggestion
LOGGER.info("Skipping temp cleanup due to -D{}=true", MAVEN_TEMPFILE_KEEP);
```
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introduce TempFileService and lifecycle cleanup participant [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet merged PR #11405:
URL: https://github.com/apache/maven/pull/11405
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introduce TempFileService and lifecycle cleanup participant for Maven 3. [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #11405:
URL: https://github.com/apache/maven/pull/11405#issuecomment-3504885648
It looks like a duplicate of #11389 ?
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introduce TempFileService and lifecycle cleanup participant [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #11405:
URL: https://github.com/apache/maven/pull/11405#discussion_r3646643210
##########
api/maven-api-core/src/main/java/org/apache/maven/api/Constants.java:
##########
@@ -739,5 +739,11 @@ public final class Constants {
*/
public static final String MAVEN_MODEL_PROCESSOR_REFERENCE_TYPE_PREFIX = "maven.model.processor.referenceType.";
+ /**
+ * System property to keep temp material for diagnostics.
+ *
+ */
+ public static final String KEEP_PROP = "maven.tempfile.keep";
Review Comment:
All 113 other constants in this file use the `MAVEN_` prefix (`MAVEN_HOME`, `MAVEN_CACHE_STATS`, `MAVEN_REPO_LOCAL`, etc.). This should be `MAVEN_TEMPFILE_KEEP`. Also missing `@since 4.1.0` and `@Config` annotation (71 other constants have `@Config`).
```suggestion
/**
* System property to keep temp material for diagnostics.
*
* @since 4.1.0
*/
@Config(type = "java.lang.Boolean", defaultValue = "false")
public static final String MAVEN_TEMPFILE_KEEP = "maven.tempfile.keep";
```
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/TempFileService.java:
##########
@@ -0,0 +1,69 @@
+/*
+ * 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.api.services;
+
+import java.io.IOException;
+import java.nio.file.Path;
+
+import org.apache.maven.api.Service;
+import org.apache.maven.api.Session;
+import org.apache.maven.api.annotations.Nonnull;
+
+/**
+ * Service to create and track temporary files/directories for a Maven build.
+ * All created paths are deleted automatically when the session ends.
+ */
+public interface TempFileService extends Service {
+
+ /**
+ * Creates a temp file in the default temp directory.
+ */
+ @Nonnull
+ Path createTempFile(Session session, String prefix, String suffix) throws IOException;
+
+ /**
+ * Creates a temp file in the given directory.
+ */
+ @Nonnull
+ Path createTempFile(Session session, String prefix, String suffix, Path directory) throws IOException;
+
+ /**
+ * Creates a temp directory in the default temp directory.
+ */
+ @Nonnull
+ Path createTempDirectory(Session session, String prefix) throws IOException;
+
+ /**
+ * Creates a temp directory in the given directory.
+ */
+ @Nonnull
+ Path createTempDirectory(Session session, String prefix, Path directory) throws IOException;
+
+ /**
+ * Registers an externally created path for cleanup at session end.
+ */
+ @Nonnull
+ void register(Session session, Path path);
Review Comment:
`@Nonnull` on a `void` return type is semantically meaningless. In existing services (`ArtifactDeployer.deploy`, `ArtifactManager.setPath`), `@Nonnull` is applied to parameters, not void returns.
Same issue on `cleanup()` below.
```suggestion
void register(@Nonnull Session session, @Nonnull Path path);
```
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introduce TempFileService and lifecycle cleanup participant for Maven 3. [maven]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on PR #11405:
URL: https://github.com/apache/maven/pull/11405#issuecomment-3506660266
> It looks like a duplicate of #11389 ?
Yes. one for master and the other for maven-4.X
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introduce TempFileService and lifecycle cleanup participant for Maven 3. [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #11405:
URL: https://github.com/apache/maven/pull/11405#discussion_r2505452329
##########
api/maven-api-core/src/main/java/org/apache/maven/api/Constants.java:
##########
@@ -739,5 +739,11 @@ public final class Constants {
*/
public static final String MAVEN_MODEL_PROCESSOR_REFERENCE_TYPE_PREFIX = "maven.model.processor.referenceType.";
+ /**
+ * System property to keep temp material for diagnostics.
+ *
+ */
+ public static final String KEEP_PROP = "maven.tempfile.keep";
Review Comment:
Rename to `MAVEN_TEMPFILE_KEEP` for consistency.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introduce TempFileService and lifecycle cleanup participant [maven]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #11405:
URL: https://github.com/apache/maven/pull/11405#discussion_r4256950270
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/TempFileService.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * 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.api.services;
+
+import java.io.IOException;
+import java.nio.file.Path;
+
+import org.apache.maven.api.Service;
+import org.apache.maven.api.Session;
+import org.apache.maven.api.annotations.Experimental;
+import org.apache.maven.api.annotations.Nonnull;
+import org.apache.maven.api.annotations.Nullable;
+
+/**
+ * Service to create and track temporary files/directories for a Maven build.
+ * All created paths are deleted automatically when the session ends.
+ *
+ * @since 4.1.0
+ */
+@Experimental
+public interface TempFileService extends Service {
+
+ /**
+ * Creates a temp file in the default temp directory.
+ *
+ * @throws IOException if the path cannot be created
+ */
+ @Nonnull
+ Path createTempFile(@Nonnull Session session, @Nullable String prefix, @Nullable String suffix) throws IOException;
Review Comment:
📝 **Missing `@param` and `@return` tags on all factory methods**
All six methods in this interface have Javadoc but are missing `@param` and `@return` documentation. Every other Maven 4 API service interface (`ArtifactResolver`, `DependencyResolver`, `VersionResolver`, etc.) consistently documents all parameters and return values with Javadoc tags.
For example, `createTempFile(Session, String, String)` should read:
```suggestion
/**
* Creates a temp file in the default temp directory.
*
* @param session the Maven session, must not be {@code null}
* @param prefix the prefix string to be used in generating the file's name; may be {@code null}
* @param suffix the suffix string to be used in generating the file's name; may be {@code null}
* @return the path to the newly created temp file, never {@code null}
* @throws IOException if the path cannot be created
*/
```
Please add `@param` and `@return` tags to all six methods (`createTempFile` ×2, `createTempDirectory` ×2, `register`, `cleanup`).
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introduce TempFileService and lifecycle cleanup participant [maven]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #11405:
URL: https://github.com/apache/maven/pull/11405#discussion_r4256304613
##########
api/maven-api-core/src/main/java/org/apache/maven/api/Constants.java:
##########
@@ -868,5 +868,17 @@ public final class Constants {
@Config(type = "java.lang.Boolean", defaultValue = "false")
public static final String MAVEN_MODEL_DEPENDENCY_INTERPOLATION_FULL = "maven.model.dependencyInterpolation.full";
+ /**
+ * System property to keep temp material for diagnostics.
+ * When set to {@code true} via {@code -Dmaven.tempfile.keep=true}, temporary files and directories
+ * created by {@link org.apache.maven.api.services.TempFileService} are not deleted at session end.
+ * This is a JVM system property only (read via {@link Boolean#getBoolean(String)});
+ * it is not resolved through Maven's property mechanism (pom.xml, profiles, settings.xml).
+ *
+ * @since 4.1.0
+ */
+ @Config(type = "java.lang.Boolean", defaultValue = "false")
Review Comment:
⚠️ **`@Config` without `source = Config.Source.SYSTEM_PROPERTIES` contradicts the Javadoc**
The Javadoc states this property is "not resolved through Maven's property mechanism (pom.xml, profiles, settings.xml)" — but `@Config` with the default `source = USER_PROPERTIES` signals to Maven's config tooling that users CAN set it through those mechanisms. In practice, anything set via `pom.xml`/`settings.xml` goes through Maven's property merger (not `System.setProperty()`), so `Boolean.getBoolean()` silently ignores it.
Choose one:
1. **Make it truly Maven-property-configurable** by replacing `Boolean.getBoolean(MAVEN_TEMPFILE_KEEP)` with session property resolution (e.g. `Boolean.parseBoolean(session.getUserProperties().get(MAVEN_TEMPFILE_KEEP))`) — and update the Javadoc accordingly.
2. **Keep it a JVM-only `-D` flag** by removing `@Config` (since it's not participting in Maven's config resolution) or adding `source = Config.Source.SYSTEM_PROPERTIES` + `readOnly = false` if a SYSTEM_PROPERTIES source without readOnly is appropriate here.
```suggestion
@Config(type = "java.lang.Boolean", defaultValue = "false", source = Config.Source.USER_PROPERTIES)
public static final String MAVEN_TEMPFILE_KEEP = "maven.tempfile.keep";
```
Note: the suggestion above keeps the annotation but makes the source explicit — the real fix is aligning the implementation with the declared source.
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/TempFileService.java:
##########
@@ -0,0 +1,75 @@
+/*
+ * 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.api.services;
+
+import java.io.IOException;
+import java.nio.file.Path;
+
+import org.apache.maven.api.Service;
+import org.apache.maven.api.Session;
+import org.apache.maven.api.annotations.Experimental;
+import org.apache.maven.api.annotations.Nonnull;
+import org.apache.maven.api.annotations.Nullable;
+
+/**
+ * Service to create and track temporary files/directories for a Maven build.
+ * All created paths are deleted automatically when the session ends.
+ *
+ * @since 4.1.0
+ */
+@Experimental
+public interface TempFileService extends Service {
+
+ /**
+ * Creates a temp file in the default temp directory.
+ */
Review Comment:
📝 **Missing `@throws` on all four factory methods and `cleanup()`**
Public API methods that declare `throws IOException` must document it. Without `@throws`, callers have no Javadoc guidance on when the exception is thrown. Same applies to the other three `create*` methods and `cleanup()`. Example fix for all five:
```suggestion
/**
* Creates a temp file in the default temp directory.
*
* @throws IOException if the file cannot be created
*/
```
##########
impl/maven-core/src/main/java/org/apache/maven/internal/impl/TempFileCleanupParticipant.java:
##########
@@ -0,0 +1,59 @@
+/*
+ * 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.internal.impl;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import org.apache.maven.AbstractMavenLifecycleParticipant;
+import org.apache.maven.api.Session;
+import org.apache.maven.api.services.TempFileService;
+import org.apache.maven.execution.MavenSession;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Hooks into the Maven lifecycle and removes all temp material after the session.
+ */
+@Named
+@Singleton
+public final class TempFileCleanupParticipant extends AbstractMavenLifecycleParticipant {
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(TempFileCleanupParticipant.class);
+
+ private final TempFileService tempFileService;
+
+ @Inject
+ public TempFileCleanupParticipant(final TempFileService tempFileService) {
+ this.tempFileService = tempFileService;
+ }
+
+ @Override
+ public void afterSessionEnd(final MavenSession mavenSession) {
+ // Bridge to the API Session (available in Maven 4).
+ final Session apiSession = mavenSession.getSession();
+ try {
+ tempFileService.cleanup(apiSession);
+ } catch (final Exception e) {
+ // We're at session end; just log. Maven already reported build result.
+ LOGGER.warn("Temp cleanup failed: {}", e.getMessage());
Review Comment:
🐛 **Stack trace lost; catch is too broad**
Two problems:
1. `e.getMessage()` logs only the exception message, discarding the full stack trace. In production, when temp cleanup fails silently at session end, the message alone ("Temp cleanup failed: null" for NPE, or a terse IOException message) makes root-cause diagnosis impossible. SLF4J supports logging the full exception: `LOGGER.warn("Temp cleanup failed", e)` (no `{}` placeholder — SLF4J appends the throwable automatically).
2. `cleanup()` only declares `throws IOException`. Catching `Exception` broadly hides any unexpected `RuntimeException` (NPE, `IllegalStateException`) under the same generic log message.
```suggestion
} catch (final IOException e) {
// We're at session end; just log. Maven already reported build result.
LOGGER.warn("Temp cleanup failed", e);
}
```
##########
impl/maven-core/src/test/java/org/apache/maven/internal/impl/DefaultTempFileServiceTest.java:
##########
@@ -0,0 +1,237 @@
+/*
+ * 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.internal.impl;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.Objects;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Supplier;
+
+import org.apache.maven.api.Constants;
+import org.apache.maven.api.Session;
+import org.apache.maven.api.SessionData;
+import org.apache.maven.api.services.TempFileService;
+import org.apache.maven.execution.MavenSession;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+/**
+ * Tests for DefaultTempFileService and TempFileCleanupParticipant.
+ */
+class DefaultTempFileServiceTest {
+
+ @AfterEach
+ void clearKeepProp() {
+ System.clearProperty(Constants.MAVEN_TEMPFILE_KEEP);
+ }
+
+ @Test
+ void createsFilesAndDirectoriesAndCleansThemUp() throws IOException {
+ final TempFileService svc = new DefaultTempFileService();
+ final SessionData data = new MapBackedSessionData();
+ final Session session = mock(Session.class);
+ when(session.getData()).thenReturn(data);
+
+ final Path f1 = svc.createTempFile(session, "tfs-", ".bin");
+ final Path d1 = svc.createTempDirectory(session, "tfs-");
+ final Path nested = Files.createTempFile(d1, "inner-", ".tmp");
+
+ assertTrue(Files.exists(f1), "temp file must exist");
+ assertTrue(Files.exists(d1), "temp dir must exist");
+ assertTrue(Files.exists(nested), "nested file must exist");
+
+ svc.cleanup(session);
+
+ assertFalse(Files.exists(f1), "temp file must be deleted");
+ assertFalse(Files.exists(d1), "temp dir tree must be deleted");
+ assertFalse(Files.exists(nested), "nested file must be deleted");
+ }
+
+ @Test
+ void registerExternalPathIsAlsoDeleted() throws IOException {
+ final TempFileService svc = new DefaultTempFileService();
+ final SessionData data = new MapBackedSessionData();
+ final Session session = mock(Session.class);
+ when(session.getData()).thenReturn(data);
+
+ final Path externalDir = Files.createTempDirectory("ext-");
+ final Path nested = Files.createTempFile(externalDir, "ext-inner-", ".tmp");
+ assertTrue(Files.exists(externalDir));
+ assertTrue(Files.exists(nested));
+
+ svc.register(session, externalDir);
+ svc.cleanup(session);
+
+ assertFalse(Files.exists(externalDir), "registered external dir must be deleted recursively");
+ assertFalse(Files.exists(nested));
+ }
+
+ @Test
+ void createsFileInSpecifiedDirectory() throws IOException {
+ final TempFileService svc = new DefaultTempFileService();
+ final SessionData data = new MapBackedSessionData();
+ final Session session = mock(Session.class);
+ when(session.getData()).thenReturn(data);
+
+ final Path customDir = Files.createTempDirectory("custom-dir-");
+ try {
+ final Path f = svc.createTempFile(session, "dir-", ".tmp", customDir);
+ assertTrue(Files.exists(f), "temp file must exist");
+ assertTrue(f.startsWith(customDir), "temp file must be inside the specified directory");
+
+ svc.cleanup(session);
+
+ assertFalse(Files.exists(f), "temp file must be deleted");
+ } finally {
+ Files.deleteIfExists(customDir);
+ }
+ }
+
+ @Test
+ void createsDirectoryInSpecifiedParent() throws IOException {
+ final TempFileService svc = new DefaultTempFileService();
+ final SessionData data = new MapBackedSessionData();
+ final Session session = mock(Session.class);
+ when(session.getData()).thenReturn(data);
+
+ final Path parentDir = Files.createTempDirectory("parent-dir-");
+ try {
+ final Path d = svc.createTempDirectory(session, "nested-", parentDir);
+ assertTrue(Files.exists(d), "temp directory must exist");
+ assertTrue(d.startsWith(parentDir), "temp directory must be inside the specified parent");
+
+ svc.cleanup(session);
+
+ assertFalse(Files.exists(d), "temp directory must be deleted");
+ } finally {
+ Files.deleteIfExists(parentDir);
+ }
+ }
+
+ @Test
+ void keepPropertySkipsCleanup() throws IOException {
+ final TempFileService svc = new DefaultTempFileService();
+ final SessionData data = new MapBackedSessionData();
+ final Session session = mock(Session.class);
+ when(session.getData()).thenReturn(data);
+
+ final Path f = svc.createTempFile(session, "keep-", ".tmp");
+ assertTrue(Files.exists(f));
+
+ System.setProperty(Constants.MAVEN_TEMPFILE_KEEP, "true");
+ svc.cleanup(session);
+
+ assertTrue(Files.exists(f), "cleanup must be skipped when -Dmaven.tempfile.keep=true");
+
+ // turn cleanup back on and verify it deletes
+ System.clearProperty(Constants.MAVEN_TEMPFILE_KEEP);
+ svc.cleanup(session);
+ assertFalse(Files.exists(f));
+ }
+
+ @Test
+ void cleanupParticipantDelegatesCleanupToService() throws IOException {
+ final TempFileService svc = new DefaultTempFileService();
+ final SessionData data = new MapBackedSessionData();
+ final Session apiSession = mock(Session.class);
+ when(apiSession.getData()).thenReturn(data);
+
+ final Path f = svc.createTempFile(apiSession, "participant-", ".tmp");
Review Comment:
⚠️ **`System.setProperty` without local try-finally — parallel execution risk**
`@AfterEach` mitigates sequential pollution but does not protect against parallel test execution (JUnit 5 allows it). If `svc.cleanup(session)` on line 164 throws, `@AfterEach` still runs — but if another test starts between the `setProperty` and the `@AfterEach` cleanup, it will see the mutated property. Guard with a `try-finally` inside the test method:
```suggestion
System.setProperty(Constants.MAVEN_TEMPFILE_KEEP, "true");
try {
svc.cleanup(session);
assertTrue(Files.exists(f), "cleanup must be skipped when -Dmaven.tempfile.keep=true");
} finally {
System.clearProperty(Constants.MAVEN_TEMPFILE_KEEP);
}
```
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introduce TempFileService and lifecycle cleanup participant [maven]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on PR #11405:
URL: https://github.com/apache/maven/pull/11405#issuecomment-6165633283
Pushed an update with the `@param` and `@return` tags on the six `TempFileService` methods that gnodet-bot asked for. They also say that `cleanup` skips the deletion when `maven.tempfile.keep` is `true`, and that a `null` suffix gives `.tmp`. The earlier points (`@Config`, `@throws`, stack traces in the log, no `System.setProperty` left in the tests) are in the previous commit. @gnodet could you take another look when you have time?
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Introduce TempFileService and lifecycle cleanup participant [maven]
Posted by "arturobernalg (via GitHub)" <gi...@apache.org>.
arturobernalg commented on PR #11405:
URL: https://github.com/apache/maven/pull/11405#issuecomment-6118469674
Rebased on master and squashed. The earlier requests are already in (`MAVEN_TEMPFILE_KEEP` with `@Config` and `@since`, no `@Nonnull` on void methods, static logger, best-effort `deleteTree`, tests for the directory overloads and the cleanup participant). One remaining nit: a test still sets the keep property through `System.setProperty`.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org