-
Notifications
You must be signed in to change notification settings - Fork 143
TrackingFileManager changes #1692
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 6 commits
fa570cc
bed34fd
e6fbd3c
945f52d
c32602d
b6167a2
469d884
0ae44d7
537b8a4
a41669d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
@@ -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)) { | ||
| 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); | ||
|
|
@@ -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(); | ||
|
||
| 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) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is there a check here and not while reading ?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
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 usingfileLock(fileChannel, 0)?According to the javadoc:
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically we lock with
size0 or 1 (so all or [0..1] bytes), based on file was just created or existed.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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)