gnodet-bot commented on code in PR #2205:
URL: https://github.com/apache/maven-resolver/pull/2205#discussion_r4251501921
##########
maven-resolver-spi/src/main/java/org/eclipse/aether/spi/io/PathProcessorSupport.java:
##########
@@ -249,18 +266,22 @@ public void close() throws IOException {
}
/**
- * Moves the source file to the target path without ever truncating the
target in place: the content visible at
- * the target path is either the old file or the complete new file, never
a partially written one.
+ * Moves the source file to the target path, preferring atomic rename over
truncating the target in place.
+ * <p>
+ * Normally the content visible at the target path is either the old file
or the complete new file, never a
+ * partially written one. This method attempts the rename a bounded number
of times ({@link #WINDOWS_MOVE_ATTEMPTS};
+ * file locking by virus scanners, indexers or concurrent readers is
transient on Windows), and if the file system
+ * refuses an atomic move (for example when source and target are on
different stores), the source is first staged
+ * into a collocated temporary file next to the target and then renamed
into place.
* <p>
- * Historically, on Windows the final move was implemented by opening the
target path with a truncating stream
- * and copying the source into it ({@link #classicCopy(Path, Path)}). That
left a window during which a
- * concurrent reader (for example a forked JVM resolving the same
artifact) or an untimely process kill would
- * observe, or durably leave behind, a truncated file at the final path -
after checksum validation has already
- * happened, so the torn content would from then on be trusted as
verified. Instead, this method attempts the
- * rename a bounded number of times ({@link #WINDOWS_MOVE_ATTEMPTS}, file
locking by virus scanners, indexers or
- * concurrent readers is transient on Windows), and if the file system
refuses an atomic move (for example when
- * source and target are on different stores), the source is first staged
into a collocated temporary file next
- * to the target and then renamed into place - the target path itself is
never opened for writing.
+ * <strong>Last-resort fallback (Windows / IDE lock):</strong> When all
rename attempts are exhausted with
+ * {@link AccessDeniedException} — typically because an IDE such as
IntelliJ keeps the target {@code .jar} open
+ * continuously — and {@link #WINDOWS_CLASSIC_COPY_FALLBACK} is {@code
true} (the default), the method falls back
+ * to a direct stream copy over the target path ({@link #classicCopy(Path,
Path)}). On Windows, opening a file for
+ * writing via a stream succeeds even if another process holds a shared
read handle, so the build completes. The
+ * atomicity guarantee is lost for this edge case: a concurrent reader may
observe a partially written file. Set
+ * the system property {@code
org.eclipse.aether.spi.io.PathProcessor.WINDOWS_CLASSIC_COPY_FALLBACK=false} to
Review Comment:
**Bug: property name mismatch between Javadoc and code.**
This Javadoc shows
`org.eclipse.aether.spi.io.PathProcessor.WINDOWS_CLASSIC_COPY_FALLBACK` (with a
dot before `WINDOWS`), but the field initializer is:
```java
System.getProperty(PathProcessor.class.getName() +
"WINDOWS_CLASSIC_COPY_FALLBACK", "true")
```
`PathProcessor.class.getName()` returns
`org.eclipse.aether.spi.io.PathProcessor` — no trailing dot — so the **actual**
system property the JVM reads is
`org.eclipse.aether.spi.io.PathProcessorWINDOWS_CLASSIC_COPY_FALLBACK`. A user
who follows this Javadoc and passes
`-Dorg.eclipse.aether.spi.io.PathProcessor.WINDOWS_CLASSIC_COPY_FALLBACK=false`
will silently have no effect.
Same gap exists in `WINDOWS_MOVE_ATTEMPTS` / `WINDOWS_MOVE_RETRY_DELAY`
(pre-existing), but since this Javadoc is the first to document the canonical
name, it's the one users will copy. Fix either:
```suggestion
* the system property {@code
org.eclipse.aether.spi.io.PathProcessorWINDOWS_CLASSIC_COPY_FALLBACK=false} to
```
or (preferred — cleaner property name) add the dot to the concatenation:
```java
System.getProperty(PathProcessor.class.getName() +
".WINDOWS_CLASSIC_COPY_FALLBACK", "true")
```
and update all three constants + the warn log accordingly.
##########
maven-resolver-spi/pom.xml:
##########
@@ -43,6 +43,10 @@
<groupId>org.apache.maven.resolver</groupId>
<artifactId>maven-resolver-api</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-api</artifactId>
Review Comment:
`slf4j-api` is being added at compile scope to the **SPI** module
(`maven-resolver-spi`), making it a transitive runtime dependency for every
consumer. `PathProcessorSupport` is an implementation class — its logging
should arguably live in `maven-resolver-impl` rather than the contract-only
SPI. Worth confirming this is intentional; if so, a brief comment in the POM
explaining the rationale would help future reviewers.
##########
maven-resolver-spi/src/main/java/org/eclipse/aether/spi/io/PathProcessorSupport.java:
##########
@@ -45,6 +48,8 @@
* @since 2.0.13
*/
public class PathProcessorSupport implements PathProcessor {
+ protected static final Logger LOGGER =
LoggerFactory.getLogger(PathProcessorSupport.class);
Review Comment:
Nit: `private static final Logger LOGGER` is the conventional visibility for
loggers — no subclass should log through a parent's logger. `protected` exposes
it unnecessarily.
--
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]
