Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import java.nio.channels.FileLock;
import java.nio.channels.OverlappingFileLockException;
import java.nio.file.Files;
import java.nio.file.NoSuchFileException;
import java.nio.file.Path;
import java.nio.file.StandardOpenOption;
import java.util.Map;
Expand Down Expand Up @@ -61,15 +62,14 @@ public Properties read(File file) {
@Override
public Properties read(Path path) {
if (Files.isReadable(path)) {
synchronized (getMutex(path)) {
try {
long fileSize = Files.size(path);
try (FileChannel fileChannel = FileChannel.open(path, StandardOpenOption.READ);
FileLock unused = fileLock(fileChannel, Math.max(1, fileSize), true)) {
Properties props = new Properties();
props.load(Channels.newInputStream(fileChannel));
return props;
}
synchronized (mutex(path)) {
try (FileChannel fileChannel = FileChannel.open(path, StandardOpenOption.READ);
FileLock unused = fileLock(fileChannel, Math.max(1, fileChannel.size()), true)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the purpose of using fileLock(fileChannel, x) ? Why not using fileLock(fileChannel, 0) ?
According to the javadoc:

A value of zero means to lock all bytes from the specified starting position to the end of the file, regardless of whether the file is subsequently extended or truncated

Copy link
Member Author

@cstamas cstamas Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically we lock with size 0 or 1 (so all or [0..1] bytes), based on file was just created or existed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's what the code does. I don't understand why. What we want is a lock on the full file, so why just requesting 1 byte instead of the whole length (which will be extended automatically when written) ?
This parameter seems useless to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And it forces to add a call to retrieve the file size...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you looked at outdated PR? As this morning I removed the 0/1 dance fully (and hence the file size call as well)

Properties props = new Properties();
props.load(Channels.newInputStream(fileChannel));
return props;
} catch (NoSuchFileException e) {
LOGGER.debug("No such file to read {}: {}", path, e.getMessage());
} catch (IOException e) {
LOGGER.warn("Failed to read tracking file '{}'", path, e);
throw new UncheckedIOException(e);
Expand All @@ -87,64 +87,71 @@ public Properties update(File file, Map<String, String> updates) {

@Override
public Properties update(Path path, Map<String, String> updates) {
Properties props = new Properties();
try {
Files.createDirectories(path.getParent());
} catch (IOException e) {
LOGGER.warn("Failed to create tracking file parent '{}'", path, e);
throw new UncheckedIOException(e);
}

synchronized (getMutex(path)) {
try {
long fileSize;
try {
fileSize = Files.size(path);
} catch (IOException e) {
fileSize = 0L;
Properties props = new Properties();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The props variable should be defined inside the try block, with the return statement moved at the end of the same block.

synchronized (mutex(path)) {
try (FileChannel fileChannel = FileChannel.open(
path, StandardOpenOption.READ, StandardOpenOption.WRITE, StandardOpenOption.CREATE);
FileLock unused = fileLock(fileChannel, Math.max(1, fileChannel.size()), false)) {
if (fileChannel.size() > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is there a check here and not while reading ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading will be unable to open absent file, here we may create it (and will be 0 byte)

props.load(Channels.newInputStream(fileChannel));
}
try (FileChannel fileChannel = FileChannel.open(
path, StandardOpenOption.READ, StandardOpenOption.WRITE, StandardOpenOption.CREATE);
FileLock unused = fileLock(fileChannel, Math.max(1, fileSize), false)) {
if (fileSize > 0) {
props.load(Channels.newInputStream(fileChannel));
}

for (Map.Entry<String, String> update : updates.entrySet()) {
if (update.getValue() == null) {
props.remove(update.getKey());
} else {
props.setProperty(update.getKey(), update.getValue());
}
for (Map.Entry<String, String> update : updates.entrySet()) {
if (update.getValue() == null) {
props.remove(update.getKey());
} else {
props.setProperty(update.getKey(), update.getValue());
}

LOGGER.debug("Writing tracking file '{}'", path);
ByteArrayOutputStream stream = new ByteArrayOutputStream(1024 * 2);
props.store(
stream,
"NOTE: This is a Maven Resolver internal implementation file"
+ ", its format can be changed without prior notice.");
fileChannel.position(0);
int written = fileChannel.write(ByteBuffer.wrap(stream.toByteArray()));
fileChannel.truncate(written);
}

LOGGER.debug("Writing tracking file '{}'", path);
ByteArrayOutputStream stream = new ByteArrayOutputStream(1024 * 2);
props.store(
stream,
"NOTE: This is a Maven Resolver internal implementation file"
+ ", its format can be changed without prior notice.");
fileChannel.position(0);
int written = fileChannel.write(ByteBuffer.wrap(stream.toByteArray()));
fileChannel.truncate(written);
} catch (IOException e) {
LOGGER.warn("Failed to write tracking file '{}'", path, e);
throw new UncheckedIOException(e);
}
}

return props;
}

private Object getMutex(Path path) {
// The interned string of path is (mis)used as mutex, to exclude different threads going for same file,
// as JVM file locking happens on JVM not on Thread level. This is how original code did it ¯\_(ツ)_/¯
/*
* NOTE: Locks held by one JVM must not overlap and using the canonical path is our best bet, still another
* piece of code might have locked the same file (unlikely though) or the canonical path fails to capture file
* identity sufficiently as is the case with Java 1.6 and symlinks on Windows.
*/
@Override
public boolean delete(File file) {
return delete(file.toPath());
}

@Override
public boolean delete(Path path) {
if (Files.isReadable(path)) {
synchronized (mutex(path)) {
try (FileChannel fileChannel = FileChannel.open(path, StandardOpenOption.WRITE);
FileLock unused = fileLock(fileChannel, Math.max(1, fileChannel.size()), false)) {
Files.delete(path);
return true;
} catch (NoSuchFileException e) {
LOGGER.debug("No such file to delete {}: {}", path, e.getMessage());
} catch (IOException e) {
LOGGER.warn("Failed to delete tracking file '{}'", path, e);
throw new UncheckedIOException(e);
}
}
}
return false;
}

private Object mutex(Path path) {
return path.toAbsolutePath().normalize().toString().intern();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@
import javax.inject.Named;
import javax.inject.Singleton;

import java.io.IOException;
import java.io.UncheckedIOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Collections;
Expand Down Expand Up @@ -482,11 +480,7 @@ public void touchArtifact(RepositorySystemSession session, UpdateCheck<Artifact,
Properties props = write(touchPath, dataKey, transferKey, check.getException());

if (Files.exists(artifactPath) && !hasErrors(props)) {
try {
Files.deleteIfExists(touchPath);
} catch (IOException e) {
throw new UncheckedIOException(e);
}
trackingFileManager.delete(touchPath);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,4 +54,20 @@ public interface TrackingFileManager {
* @since 2.0.0
*/
Properties update(Path path, Map<String, String> updates);

/**
* Deletes the specified properties file, if exists. If file existed and was deleted, returns {@code true}.
*
* @deprecated Use {@link #delete(Path)} instead.
* @since 1.9.25
*/
@Deprecated
boolean delete(File file);

/**
* Deletes the specified properties file, if exists. If file existed and was deleted, returns {@code true}.
*
* @since 2.0.14
*/
boolean delete(Path path);
}