Skip to content

fix: attempt to read without locking when locking fails; Some file sy… #205

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.

Sign up for GitHub

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

Merged
bogovicj merged 11 commits into from
Feb 11, 2026
Merged

fix: attempt to read without locking when locking fails; Some file sy… #205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
04c1568
fix: attempt to read without locking when locking fails; Some file sy…
cmhulbert Feb 5, 2026
84f9475
feat: support atomic, unsafe, and fallback policies for file system r…
cmhulbert Feb 10, 2026
289c607
feat: kva.createReadData throws N5NoSuchKeyException for missing keys…
cmhulbert Feb 10, 2026
795e96a
chore: deprecate `lockForReading/Writing` methods in favor of `create…
cmhulbert Feb 10, 2026
164c13b
fix(test): rename CodecSerialization so it's name matches he surefire…
cmhulbert Feb 10, 2026
760e5b6
refactor(test): move lock specific tests to it's own file
cmhulbert Feb 10, 2026
526d0fb
feat: don't throw on NoSuchFileException for delete
cmhulbert Feb 10, 2026
434fbe4
BREAKING: remove `fileSystem` parameter/field from FileSystemKeyValue…
cmhulbert Feb 10, 2026
4f8bd5d
cleanup(test): cleanup uses of old FileSystemKVA constructor
cmhulbert Feb 10, 2026
5d72e38
fix: root patch may be null, get separator from fsPath
cmhulbert Feb 10, 2026
6a57f57
feat: make IoPolicy public for use in other KVA implementations
cmhulbert Feb 11, 2026
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
140 changes: 76 additions & 64 deletions src/main/java/org/janelia/saalfeldlab/n5/FileSystemKeyValueAccess.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,25 +28,16 @@
*/
package org.janelia.saalfeldlab.n5;

import java.nio.file.StandardOpenOption;
import java.io.*;
import java.nio.file.*;

import org.janelia.saalfeldlab.n5.N5Exception.N5IOException;
import org.janelia.saalfeldlab.n5.N5Exception.N5NoSuchKeyException;

import java.io.File;
import java.io.IOException;
import java.io.UncheckedIOException;
import java.nio.ByteBuffer;
import java.nio.channels.FileChannel;
import java.net.URI;
import java.net.URISyntaxException;
import java.nio.file.DirectoryNotEmptyException;
import java.nio.file.FileAlreadyExistsException;
import java.nio.file.FileSystem;
import java.nio.file.FileSystemException;
import java.nio.file.Files;
import java.nio.file.NoSuchFileException;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.nio.file.attribute.FileAttribute;
import java.util.Arrays;
import java.util.Comparator;
Expand All @@ -68,49 +59,62 @@
*/
public class FileSystemKeyValueAccess implements KeyValueAccess {

protected final FileSystem fileSystem;

@cmhulbert cmhulbert Feb 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

remove fileSystem field. It was used inconsistently. Most file calls used Files or Paths which get the file system internally


/**
* Opens a {@link FileSystemKeyValueAccess} with a {@link FileSystem}.
*
* @param fileSystem the file system
*/
public FileSystemKeyValueAccess(final FileSystem fileSystem) {

this.fileSystem = fileSystem;
private static final IoPolicy ioPolicy = getIoPolicy();

@cmhulbert cmhulbert Feb 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

relevant code for configuring what I'm calling IoPolicy. currently support strict atomic, no locking attempt unsafe and fallback which tries with atomic, but tries again with unsafe if it fails.


private static IoPolicy getIoPolicy() {
String property = System.getProperty("n5.ioPolicy");

@bogovicj bogovicj Feb 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Longer term, I'd prefer that this property be configurable at a per-KVA instance (or maybe per KVA-class [1]) level rather than at a per-JVM level.

That being said, it won't bother me too much if this is how we do it for this release.

if (property == null)
return FsIoPolicy.atomicWithFallback;

switch (property) {
case "atomic":
return new FsIoPolicy.Atomic();
case "unsafe":
return new FsIoPolicy.Unsafe();
case "atomicFallbackUnsafe":
default:
return FsIoPolicy.atomicWithFallback;
}
}

@Override
public VolatileReadData createReadData(final String normalPath) {
public VolatileReadData createReadData(final String normalPath) throws N5IOException {

try {
return VolatileReadData.from(new FileLazyRead(fileSystem.getPath(normalPath)));
} catch (N5NoSuchKeyException e) {
return null;
return ioPolicy.read(normalPath);
} catch (final NoSuchFileException e) {
throw new N5NoSuchKeyException("No such file", e);
} catch (IOException | UncheckedIOException e) {
throw new N5IOException("Failed to lock file for reading: " + normalPath, e);
}
}

@Override
public void write(final String normalPath, final ReadData data) throws N5IOException {
final Path path = fileSystem.getPath(normalPath);
try (final LockedFileChannel channel = lockForWriting(path)) {
data.writeTo(channel.newOutputStream());
} catch (IOException e) {
throw new N5IOException(e);
try {
ioPolicy.write(normalPath, data);
} catch (final NoSuchFileException e) {
throw new N5NoSuchKeyException("No such file", e);
} catch (IOException | UncheckedIOException e) {
throw new N5IOException("Failed to lock file for writing: " + normalPath, e);
}
}

@Override
@Deprecated
public LockedFileChannel lockForReading(final String normalPath) throws N5IOException {

return lockForReading(fileSystem.getPath(normalPath));
return lockForReading(Paths.get(normalPath));
}

@Override
@Deprecated
public LockedChannel lockForWriting(final String normalPath) throws N5IOException {

return lockForWriting(fileSystem.getPath(normalPath));
return lockForWriting(Paths.get(normalPath));
}

@Deprecated
protected static LockedFileChannel lockForReading(final Path path) throws N5IOException {

try {
Expand All @@ -122,6 +126,7 @@ protected static LockedFileChannel lockForReading(final Path path) throws N5IOEx
}
}

@Deprecated
static LockedFileChannel lockForWriting(final Path path) throws N5IOException {

try {
Expand All @@ -136,45 +141,45 @@ static LockedFileChannel lockForWriting(final Path path) throws N5IOException {
@Override
public boolean isDirectory(final String normalPath) {

final Path path = fileSystem.getPath(normalPath);
final Path path = Paths.get(normalPath);
return Files.isDirectory(path);
}

@Override
public boolean isFile(final String normalPath) {

final Path path = fileSystem.getPath(normalPath);
final Path path = Paths.get(normalPath);
return Files.isRegularFile(path);
}

@Override
public boolean exists(final String normalPath) {

final Path path = fileSystem.getPath(normalPath);
final Path path = Paths.get(normalPath);
return Files.exists(path);
}

@Override
public long size(final String normalPath) {

return size(fileSystem.getPath(normalPath));
return size(Paths.get(normalPath));
}

protected static long size(final Path path) {

try {
return Files.size(path);
} catch (NoSuchFileException e) {
throw new N5Exception.N5NoSuchKeyException("No such file", e);
throw new N5NoSuchKeyException("No such file", e);
} catch (IOException | UncheckedIOException e) {
throw new N5Exception.N5IOException(e);
throw new N5IOException(e);
}
}

@Override
public String[] listDirectories(final String normalPath) throws N5IOException {

final Path path = fileSystem.getPath(normalPath);
final Path path = Paths.get(normalPath);
try (final Stream<Path> pathStream = Files.list(path)) {
return pathStream
.filter(Files::isDirectory)
Expand All @@ -190,7 +195,7 @@ public String[] listDirectories(final String normalPath) throws N5IOException {
@Override
public String[] list(final String normalPath) throws N5IOException {

final Path path = fileSystem.getPath(normalPath);
final Path path = Paths.get(normalPath);
try (final Stream<Path> pathStream = Files.list(path)) {
return pathStream
.map(a -> path.relativize(a).toString())
Expand All @@ -205,8 +210,9 @@ public String[] list(final String normalPath) throws N5IOException {
@Override
public String[] components(final String path) {

final Path fsPath = fileSystem.getPath(path);
final Path fsPath = Paths.get(path);
final Path root = fsPath.getRoot();
final String separator = fsPath.getFileSystem().getSeparator();
final String[] components;
int o;
if (root == null) {
Expand All @@ -222,7 +228,6 @@ public String[] components(final String path) {
String name = fsPath.getName(i - o).toString();
/* Preserve trailing slash on final component if present*/
if (i == components.length - 1) {
final String separator = fileSystem.getSeparator();
final String trailingSeparator = path.endsWith(separator) ? separator : path.endsWith("/") ? "/" : "";
name += trailingSeparator;
}
Expand All @@ -234,7 +239,7 @@ public String[] components(final String path) {
@Override
public String parent(final String path) {

final Path parent = fileSystem.getPath(path).getParent();
final Path parent = Paths.get(path).getParent();
if (parent == null)
return null;
else
Expand All @@ -244,8 +249,8 @@ public String parent(final String path) {
@Override
public String relativize(final String path, final String base) {

final Path basePath = fileSystem.getPath(base);
return basePath.relativize(fileSystem.getPath(path)).toString();
final Path basePath = Paths.get(base);
return basePath.relativize(Paths.get(path)).toString();
}

/**
Expand All @@ -260,7 +265,7 @@ public String relativize(final String path, final String base) {
@Override
public String normalize(final String path) {

return fileSystem.getPath(path).normalize().toString();
return Paths.get(path).normalize().toString();
}

@Override
Expand All @@ -283,8 +288,8 @@ public String compose(final String... components) {
if (components == null || components.length == 0)
return null;
if (components.length == 1)
return fileSystem.getPath(components[0]).toString();
return fileSystem.getPath(components[0], Arrays.copyOfRange(components, 1, components.length)).normalize().toString();
return Paths.get(components[0]).toString();
return Paths.get(components[0], Arrays.copyOfRange(components, 1, components.length)).normalize().toString();
}

@Override public String compose(URI uri, String... components) {
Expand All @@ -307,7 +312,7 @@ public String compose(final String... components) {
public void createDirectories(final String normalPath) throws N5IOException {

try {
createDirectories(fileSystem.getPath(normalPath));
createDirectories(Paths.get(normalPath));
} catch (NoSuchFileException e) {
throw new N5NoSuchKeyException("No such file", e);
} catch (IOException | UncheckedIOException e) {
Expand All @@ -319,25 +324,23 @@ public void createDirectories(final String normalPath) throws N5IOException {
public void delete(final String normalPath) throws N5IOException {

try {
final Path path = fileSystem.getPath(normalPath);
final Path path = Paths.get(normalPath);

if (Files.isRegularFile(path))
try (final LockedChannel channel = lockForWriting(path)) {
Files.delete(path);
}
ioPolicy.delete(normalPath);
else {
try (final Stream<Path> pathStream = Files.walk(path)) {
for (final Iterator<Path> i = pathStream.sorted(Comparator.reverseOrder()).iterator(); i.hasNext();) {
for (final Iterator<Path> i = pathStream.sorted(Comparator.reverseOrder()).iterator(); i.hasNext(); ) {
final Path childPath = i.next();
if (Files.isRegularFile(childPath))
try (final LockedChannel channel = lockForWriting(childPath)) {
Files.delete(childPath);
}
ioPolicy.delete(childPath.toString());
else
tryDelete(childPath);
}
}
}
} catch (NoSuchFileException ignore) {

@cmhulbert cmhulbert Feb 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this is new; previously we threw an exception on NoSuchFile. My opinion is we should just return without complaining, since the file does not exist, which is the intention of delete.

/* It doesn't exist; that's sufficient for us to not complain on a `delete` call */
} catch (IOException | UncheckedIOException e) {
throw new N5IOException("Failed to delete file at " + normalPath, e);
}
Expand All @@ -346,7 +349,7 @@ public void delete(final String normalPath) throws N5IOException {
protected static void tryDelete(final Path path) throws IOException {

try {
Files.delete(path);
ioPolicy.delete(path.toString());
} catch (final DirectoryNotEmptyException e) {
/*
* Even though path is expected to be an empty directory, sometimes
Expand All @@ -356,7 +359,7 @@ protected static void tryDelete(final Path path) throws IOException {
try {
/* wait and reattempt */
Thread.sleep(100);
Files.delete(path);
ioPolicy.delete(path.toString());
} catch (final InterruptedException ex) {
e.printStackTrace();
Thread.currentThread().interrupt();
Expand Down Expand Up @@ -499,14 +502,23 @@ protected static void createAndCheckIsDirectory(
}
}

private static class FileLazyRead implements LazyRead {
static class FileLazyRead implements LazyRead {

private static final Closeable NO_OP = () -> { };

private final Path path;
private LockedFileChannel lock;
private Closeable lock;

FileLazyRead(final Path path) throws IOException {
this(path, true);
}

FileLazyRead(final Path path) {
FileLazyRead(final Path path, final boolean requireLock ) throws IOException {
this.path = path;
lock = lockForReading(path);
if (requireLock)
lock = FILE_LOCK_MANAGER.lockForReading(path);
else
lock = NO_OP;
}

@Override
Expand Down
61 changes: 61 additions & 0 deletions src/main/java/org/janelia/saalfeldlab/n5/FsIoPolicy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
package org.janelia.saalfeldlab.n5;

import org.janelia.saalfeldlab.n5.readdata.ReadData;
import org.janelia.saalfeldlab.n5.readdata.VolatileReadData;

import java.io.IOException;
import java.nio.channels.FileChannel;
import java.nio.file.*;

import static org.janelia.saalfeldlab.n5.FileKeyLockManager.FILE_LOCK_MANAGER;

public class FsIoPolicy {

static final IoPolicy atomicWithFallback = IoPolicy.withFallback(new Atomic(), new Unsafe());

public static class Unsafe implements IoPolicy {
@Override
public void write(String key, ReadData readData) throws IOException {
final Path path = Paths.get(key);
Files.copy(readData.inputStream(), path, StandardCopyOption.REPLACE_EXISTING);
}

@Override
public VolatileReadData read(final String key) throws IOException {
final Path path = Paths.get(key);
FileSystemKeyValueAccess.FileLazyRead fileLazyRead = new FileSystemKeyValueAccess.FileLazyRead(path, false);
return VolatileReadData.from(fileLazyRead);
}

@Override
public void delete(final String key) throws IOException {
final Path path = Paths.get(key);
Files.deleteIfExists(path);
}
}

public static class Atomic implements IoPolicy {
@Override
public void write(String key, ReadData readData) throws IOException {
final Path path = Paths.get(key);
try (LockedFileChannel channel = FILE_LOCK_MANAGER.lockForWriting(path)) {
readData.writeTo(channel.newOutputStream());
}
}

@Override
public VolatileReadData read(String key) throws IOException {
final Path path = Paths.get(key);
FileSystemKeyValueAccess.FileLazyRead fileLazyRead = new FileSystemKeyValueAccess.FileLazyRead(path, true);
return VolatileReadData.from(fileLazyRead);
}

@Override
public void delete(final String key) throws IOException {
final Path path = Paths.get(key);
try (LockedFileChannel ignore = FILE_LOCK_MANAGER.lockForWriting(path)) {
Files.delete(path);
}
}
}
}
Loading