fix: fetch snapshotter proxy object without holding cache lock

[austinvazquez]

Signed-off-by: Austin Vazquez [email protected]

Issue #, if available:
None

Description of changes:
fetch operation should not occur while the cache lock is held.

This unblocks snapshot requests whom have dialed their microVM to continue in the edge case where the lock has been acquired by a slow dialer.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[ginglis13]

we use a reader lock above. Should we be doing that here too?

[austinvazquez]

Oof that adds a new level of complexity if we need to account for a second double check lock.

[kzys]

I'm confused. If !ok, wouldn't cache.snapshotters[key] be always nil?

[austinvazquez]

@kzys, unless another thread populates the cache after we have a cache miss but before we acquire the writers lock. The cache entry needs to be .Close() before it is garbage collected to cleanup system resources for metrics proxy.

[austinvazquez]

But @ginglis13 is correct this solution won't work for edge cases because we can leak now. So it requires fetch after reader's lock cache miss but before writer's lock acquisition.

[kzys]

How about moving cache.mutex.RUnlock() in this if block? The snapshotter doesn't do much time consuming operations between RUnlock and Lock.

[austinvazquez]

That may result in deadlock. We'd block on Lock without releasing RUnlock. I don't believe RWMutex has brains to know assign writer's lock even if only one reader's lock is acquired by the same thread.

[Kern--]

I think this deserves an issue to discuss options. We need to serialize dictionary writes, but you're right that we don't need to block all remote snapshotters while we dial just any snapshotter. We probably need something smarter than a single cache mutex.

[austinvazquez]

@kzys @Kern-- @ginglis13 I have created #687 to discuss options. Closing this PR for now and will re-opened based on our conversations there.