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

[cmhulbert]

See #204 for more details.

[cmhulbert]

@tpietzsch currently this handles the reading case in a way I'm happy with.

However, I think the batching branch is missing an atomic write method that uses VolatileReadData. perhaps a void write(String path, ReadData readData); method? If we have that, than we can apply similar logic there to attempt writing without locks. That may be trickier, in that we don't want to write if someone already has the lock, just if the operation is not supported.

another this I did is remove the try / catch / return null for createReadData. It can't return null if we use it in try-with-resource blocks or you get a NPE. And we DO handle already N5NoSuchKeyException and return null in places that call createReadData so we should just the N5NoSuchKeyException bubble up to the ultimate caller.

[tpietzsch]

another this I did is remove the try / catch / return null for createReadData. It can't return null if we use it in try-with-resource blocks or you get a NPE. And we DO handle already N5NoSuchKeyException and return null in places that call createReadData so we should just the N5NoSuchKeyException bubble up to the ultimate caller.

That isn't true. There will be no NPE in try-with-resources if the Closable is null.
At least this works for me:

public class ClosableNPETest {
	static class MyClosable implements AutoCloseable {
		@Override
		public void close() {
			System.out.println("MyClosable.close");
		}
	}

	static MyClosable create() {
//		return new MyClosable();
		return null; // this works fine
	}

	public static void main(String[] args) {
		try (MyClosable c = create()) {
			System.out.println("doing something with " + c);
		}
		System.out.println("done");
	}
}

[tpietzsch]

Why the boxing? This should be boolean instead of Boolean.

[cmhulbert]

No good reason, just didn't notice. I agree

[cmhulbert]

another this I did is remove the try / catch / return null for createReadData. It can't return null if we use it in try-with-resource blocks or you get a NPE. And we DO handle already N5NoSuchKeyException and return null in places that call createReadData so we should just the N5NoSuchKeyException bubble up to the ultimate caller.

That isn't true. There will be no NPE in try-with-resources if the Closable is null. At least this works for me:

public class ClosableNPETest {
	static class MyClosable implements AutoCloseable {
		@Override
		public void close() {
			System.out.println("MyClosable.close");
		}
	}

	static MyClosable create() {
//		return new MyClosable();
		return null; // this works fine
	}

	public static void main(String[] args) {
		try (MyClosable c = create()) {
			System.out.println("doing something with " + c);
		}
		System.out.println("done");
	}
}

Your example is too trivial. The NPE arrises because the block is still executed. in your example, you are only using c as String + null which is fine. but if you try to call a method on c in the block, you will get an NPE, and that is always the case that we want to use methods on the Closable returned from the createReadData method.

modifying your example, and the output:

public static void main(String[] args) {
	try (MyClosable c = create()) {
		System.out.println("doing something with " + c);
		c.toString();
	}
	System.out.println("done");
}

doing something with null
Exception in thread "main" java.lang.NullPointerException: Cannot invoke "Object.toString()" because "c" is null
	at org.janelia.saalfeldlab.n5.N5Writer$ClosableNPETest.main(N5Writer.java:485)

[tpietzsch]

Oh, then I misunderstood.

It can't return null if we use it in try-with-resource blocks or you get a NPE.

I thought that you mean the try-with-resources will try to close() the null. Instead you just meant

It can't return null if we use it in try-with-resource blocks or you get a NPE.

But then there is no problem.
The null behaviour is documented

n5/src/main/java/org/janelia/saalfeldlab/n5/shard/PositionValueAccess.java

Lines 53 to 55 in 95d70d9

	* If the requested key does not exist, either {@code null} is returned or a
	* lazy {@code VolatileReadData} that will throw {@code N5NoSuchKeyException}
	* when trying to materialize.

and I explicitly decided to do it this way ("this way" meaning that N5NoSuchKeyException will not be thrown by createReadData.) I considered the alternative, and code like this:

n5/src/main/java/org/janelia/saalfeldlab/n5/shard/DefaultDatasetAccess.java

Lines 192 to 199 in 95d70d9

	final ReadData modifiedData;
	try (final VolatileReadData existingData = pva.get(key)) {
	modifiedData = writeBlockRecursive(existingData, dataBlock, position, grid.numLevels() - 1);
	// Here, we are about to write the shard data, but with the new block modified.
	// Need to make sure that the read operations happen now before pva.set acquires a write lock
	modifiedData.materialize();
	}
	pva.set(key, modifiedData);

would become a lot more convoluted. (The code in the try block needs to run, even if there is no existingReadData.)

If we decide to let pva.get(key) throw the N5NoSuchKeyException, then the easiest solution would be to just wrap it in a static method that turns it into null, replicating the current behaviour for ease of use in DefaultDatasetAccess. That seems pointless.

[bogovicj]

We (@cmhulbert , @tpietzsch ) discussed having PositionValueAccess catching the NoSuchKeyException from the wrapped KeyValueAccess and returning null

[cmhulbert]

KVA throws N5NoSuchFileException, so we catch/return null for PositionKVA

[cmhulbert]

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.

[cmhulbert]

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

[cmhulbert]

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.

[cmhulbert]

deprecated lockForReading, lockForWriting

[bogovicj]

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.