fix an annoying bug that when regularly calling invalidateAll() on a …

[StephanPreibisch]

…SoftRefCache, it throws a NullPointerException. Somehow "ref" can be null, check line 213 while get() is still being called

[StephanPreibisch]

let's maybe discuss this more @tpietzsch @axtimwalde ?

[tpietzsch]

Just to understand better:
The invalidataAll() javadoc says: "There must be no concurrent get() operations. This may result in cache corruption and/or a deadlock."
Did you make sure of this and are still getting this bug?

[axtimwalde]

We realized that but didn't find a way to synchronize it. I.e. we have to clear the cache while a user draws in BDV. BDV renders prior requests asynchronously, and that interferes with `invalidateAll`. The best way to address this IMO would be to have a delayed call method in BDV that synchronizes with any rendering. I.e. wait for BDV to finish rendering, then do the call, then continue rendering. That would be specific to BDV though, and the next independent application that consumes something using the cache asynchronously will run into the same issue. Curious what you think would be best here.

…

On March 25, 2023 7:15:00 AM EDT, Tobias Pietzsch ***@***.***> wrote: External Email: Use Caution Just to understand better: The `invalidataAll()` javadoc says: "*There must be no concurrent get() operations. This may result in cache corruption and/or a deadlock.*" Did you make sure of this and are still getting this bug? -- Reply to this email directly or view it on GitHub: #22 (comment) You are receiving this because you were mentioned. Message ID: ***@***.***>

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[tpietzsch]

I think it either needs to be specific to the application, or all cache accesses must be synchronized.

I would make a Lock that BDV acquires/releases before/after rendering, and that the invalidating code would also acquire/release.

Potentially, the Lock could be created by the cache and locking be done automatically in invalidateAll(). But that would be complicated by having multiple caches, caches layered on top of each other, etc. I think it makes more sense to build the locking completely outside.

To not make it super-specific in the BDV API we could add beforeRender(Runnable) and afterRender(Runnable) methods (where one could pass Runnables that acquire/release the lock).

[StephanPreibisch]

Hi, for now I made a local copy of SoftRefLoaderCache called RobustSoftRefLoaderCache (containing the change above) in hot-knife which temporarily solves the issue.

How should we take it from here?

[axtimwalde]

I think it would be best to have an invokeAfterRender(Runnable) method in BDV that is called immediately if no rendering happens or immediately after rendering finishes and before the next rendering starts. That wouldn't require any additional synchronization.

[NicoKiaru]

@axtimwalde but what if several renderers exist ?

[tpietzsch]

@axtimwalde but what if several renderers exist ?

Yes, very good point. That would be a problem.
I would go with the beforeRender(Runnable) and afterRender(Runnable) then.
The lock should then be ReentrantReadWriteLock so that multiple renderers can take read locks at the same time. (And invalidateAll would take the write lock)

[tpietzsch]

@StephanPreibisch

How should we take it from here?

I'll implement the beforeRender(Runnable) and afterRender(Runnable) in BDV, then build around that in hotknife. Can you point me to where the invalidateAll() happens and something I can run to test it?

[StephanPreibisch]

Here is the code: https://github.com/saalfeldlab/hot-knife/blob/ee328b3eb0e3d357fdcadba220a6223d02d29a9c/src/main/java/org/janelia/saalfeldlab/hotknife/tools/HeightFieldBrushController.java#L93

[StephanPreibisch]

And the parameters for running PaintHeightField.java:
--n5Path=/nrs/hess/render/export/hess.n5 --n5FieldPath=/nrs/hess/render/export/hess.n5 --n5Raw=/render/wafer_52_cut_00030_to_00039/slab_013_all_align_t2_ic___20230328_105504 --n5Field=/heightfields/wafer_52_cut_00030_to_00039/slab_013_all_align_t2_ic___20230328_105504/s1/min --n5FieldOutput=/heightfields_fix/wafer_52_cut_00030_to_00039/slab_013_all_align_t2_ic___20230328_105504/min --scale=2,2,1 --offset=0 --multiSem
Guess it's best to make a local copy of the heighfield ... or just ask me to test it ...