store: replace Modified+Load with ReloadIfChanged

[giuseppe]

simplify the pattern if store.Modified() => store.Load() with a single function call store.ReloadIfChanged().

In addition to the code simplification, it solves a race condition that when only a RLock() is hold and that multiple goroutines could
run the sequence store.Modified() and store.Load(). In this case only one goroutine would see store.Modified( and run store.Load().
Other goroutines could access the old data while the goroutine that saw the change modifies the store data.

Closes: #880

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

[rhatdan]

Nice Fix
LGTM

[nalind]

The diagnosis at #880 (comment) looks good, but I'm missing how this keeps one goroutine that's using the data without having taken the loadMut mutex from having the slice it's iterating over replaced, while it's running and unaware, by another goroutine that has the mutex.

[giuseppe]

The diagnosis at #880 (comment) looks good, but I'm missing how this keeps one goroutine that's using the data without having taken the loadMut mutex from having the slice it's iterating over replaced, while it's running and unaware, by another goroutine that has the mutex.

that is protected by the other locking mechanisms we have. A goroutine that is accessing the data must already hold a RO or a RW lock. A goroutine checks for ReloadIfChanged() while holding the RO lock. No changes could have happened to the storage between a goroutine got the RO lock and it accesses the data because another goroutine/process needed to get the RW lock before it could make any change. Since a goroutine is already holding a RO lock (thus blocking other writers), it means that any other goroutine getting into the ReloadIfChanged() code block won't hit the r.Modified() condition.

The issue I've seen only happens when there are multiple goroutines checking r.Modified() together and not syncing on the Load() that only one goroutine does.

Goroutine 1	Process 2	Goroutine 3
GetROLock()
ReloadIfChanged() [might trigger a Load()]	GetRWLock()
[use the store data]	[wait]	GetROLock()
[use the store data]	[wait]	ReloadIfChanged()

ReloadIfChanged() should never cause a Load() because goroutine 1 already checked while holding the RO lock thus blocking Process 2.

One possible race condition though is that we attempt the Load also on errors:

	modified, err := r.Modified()
	if err != nil || modified {
		return r.Load()
	}

A temporary error could cause the race as it calls Load() while there are other goroutines running. I've pushed a new version where I change the condition to err == nil && modified

[giuseppe]

does the explanation above make sense? Are there other cases where a race can happen?

[nalind]

I was thinking of the case where one thread is calling a method like store.Images(), which takes a read lock, but another is in a method like store.ImageSize(), which also takes a read lock, and one of them alters the data the object is holding while the other is iterating through it. It's not exclusive to this pair of methods, but that's a window I don't see us closing here.

[giuseppe]

I was thinking of the case where one thread is calling a method like store.Images(), which takes a read lock, but another is in a method like store.ImageSize(), which also takes a read lock, and one of them alters the data the object is holding while the other is iterating through it. It's not exclusive to this pair of methods, but that's a window I don't see us closing here.

That is the case I've tried to explain with the table.

If one thread is calling store.Images() while it holds a read lock, it already went through the ReloadIfChanged() check. Holding the RO lock prevents other processes to modify the store. Another thread that gets the RO lock and gets into ReloadIfChanged(), will either 1) wait that for the first thread to complete ReloadIfChanged(), then Modified() returns false 2) don't block on the lock but Modified() still returns false so it doesn't reload the store while the first thread is already accessing its data.
This assumption is based on the fact that any reload is made while holding the RO so other threads/processes cannot modify the store in the meanwhile. The store could be modified only when there are no RO locks in place.

[nalind]

Hmm, okay, that makes sense. LGTM.

[vrothberg]

@haircommander, that should fix the issue in CRI-O