support/storage: Make the on-disk cache thread-safe.

[Shaptic]

What

When caching files, add a <name>.lock file that exists until the file is fully written. When retrieving files, check that they don't have a corresponding .lock file before reading them from the cache.

Why

This adds thread safety to the on-disk cache and is necessary for both #4468 and #4526.

Known limitations

n/a

[sreuland]

should this attempt to access local cache file be guarded by stateErr != nil? these local disk cache files could be getting re-written by the async preload process concurrently at same time?

[Shaptic]

b.lru.Get will only check the in-memory LRU cache that will tell us if localPath might be on disk, so it's a safe check. It doesn't touch the on-disk file.

[sreuland]

rather than a lock file per each cache file, wdyt about courser lock scope and intra-process for preload exec, it would cover loading of the entire cache as one atomic thing, and letting the caller coordinate it by pass the lock:

func MakeOnDiskCache(upstream Storage, dir string, maxFiles uint, preloadSemaphore *bool) (Storage, error) 

then could replace os.Stat(nameLockfile(localPath)) with atomic.LoadBool(preloadSemaphore)

was mentioning intra-process for the preload, because it doesn't seem to add value for for externalizing preload as another o/s process(requiring file lock) rather, the preload can be a goroutine launched in the web server process at startup and passes the semaphore from that to here. works in bare-metal or containerized environments with less config(i.e. no need to define a second container in k8s deployment pod to run the preload)

[Shaptic]

So I'm a little hazy on the details on what you're proposing here, but let me offer up a justification for the fine-grained locking I did here.

First off, this part is intentionally separate from the add'l code that will be necessary for the preload task, because I wanted to (a) keep things decoupled and (b) this needs thread safety for #4468 (parallel fetches) as well. I'm seeing issues in my draft PR with reads interfering with parallel fetches because of the caching layer (i.e. parallel fetches are still writing to the cache, but the GetLedger() notices a cache hit so it tries to read). That's why the locking is fine-grained on a per-file basis: parallel downloads means some files are safe while multiple may be unsafe.

I agree that the preload itself can be a goroutine, but that's distinct from needing the cache to be thread-safe for simultaneous cache read/write. Maybe the semaphore aspect can come in that next PR and act as a layer on top of the per-file locking here?

[sreuland]

@Shaptic , got it, the cache thread safety design needs granularity that works at both extremes, i.e. whole cache for preload and concurrently per cache key/entry for web requests. I see how the file lock mechanism encapsulated inside here will work for both those extremes.

I played around with a reader/writer pattern with mutexes as alternative to per-file locks for concurrency. In this approach, both the web requests and preload threads sync across concurrent read locks and exclusive write locks on each cache key/entry from the same singleton instance of OnDiskCache.

I drew it out in this sequence diagram

the net effect on sync behavior should be the same, it's just a matter of how much thrash do we think is avoided by doing memory locks vs. file system locks, lmk if you think it's worth exploring this.

the OnDiskCache interface here stays the same, but, if it's used, on that follow-on PR I do as part of #4526 would be to have the web server main create one instance of OnDiskCache and pass it to Archive for all web request threads and same instance passed to preload BuildCache thread, rather than letting each of them create their own instance.

[Shaptic]

The benefit of file-based locks is that there's no need for read locks - we get that "for free" from the OS (concurrent reads of a file is always safe), and similarly there's no need for a singleton because it's all on disk and visible to everyone.

And also the biggest benefit is that the work is already done 😆 imo we should punt exploring memory-based synchronization till later unless we see issues with this. The latency is going to be dominated by ledger and index downloads, anyway.