Allow concurrent calls for Fake methods. #16
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/S
fake.go
Outdated
| ) | ||
|
|
||
| // Fake is a fake implementation of Interface | ||
| type Fake struct { | ||
| nftContext | ||
| // lock is used to protect Table and LastTransaction. | ||
| // When Table and LastTransaction are accessed directly, it is caller's responsibility | ||
| // to ensure there is no data race. |
There was a problem hiding this comment.
That's no good... should probably make them private members, and add accessors for them that do proper locking.
There was a problem hiding this comment.
I didn't want to break existing users, but if you think it is ok, I can make them private
There was a problem hiding this comment.
later discussed on slack; just adding accessors doesn't work because you'd have to deep-copy Table. So maybe it would make more sense to expose the locking to the caller. Make the sync.Mutex an embedded field like the nftContext, and document the fact that you need to .RLock() it if you are going to look directly at Table.
Maybe .LastTransaction should be changed to .GetLastTransaction() though, with locking. (The transaction itself would be read-only at this point, so you don't have to worry about locking after you access the pointer.)
There was a problem hiding this comment.
made lock methods external, added little comments
There was a problem hiding this comment.
Not sure I like that idea of GetLastTransaction because it
- is inconsistent with Table access, which is a bit strange
- will limit potential changes in the future if we ever need to make LastTransaction not-read-only.
wdyt?
fake.go
Outdated
| ) | ||
|
|
||
| // Fake is a fake implementation of Interface | ||
| type Fake struct { | ||
| nftContext | ||
| // mutex is used to protect Table and LastTransaction. | ||
| // When Table and LastTransaction are accessed directly, the caller must acquire Fake.Rlock |
There was a problem hiding this comment.
| // When Table and LastTransaction are accessed directly, the caller must acquire Fake.Rlock | |
| // When Table and LastTransaction are accessed directly, the caller must acquire Fake.RLock |
and below
fake.go
Outdated
| // Make sure to acquire Fake.Rlock before accessing Table in a concurrent environment. | ||
| LastTransaction *Transaction |
There was a problem hiding this comment.
| // Make sure to acquire Fake.Rlock before accessing Table in a concurrent environment. | |
| LastTransaction *Transaction | |
| // Make sure to acquire Fake.Rlock before accessing LastTransaction in a concurrent environment. | |
| LastTransaction *Transaction |
But that feels wrong anyway... LastTransaction is inherently non-concurrency-safe; you can only know that it has the transaction you want if you already know for sure what the most recent Run() call was. So maybe just add a warning about that. "This is probably not useful if Interface is being used concurrently." ?
And I guess that's an argument against GetLastTransaction().
There was a problem hiding this comment.
I think it may be useful for concurrent usage also.
Let's say there is a controller thread running transactions, and test thread checking if it is correct.
On every change I make explicitly in the test, controller is expected to make exactly one transaction, but it may take time to reconcile. I will periodically check from the test thread LastTransaction until I see expected transaction, but to make it non-racy, I need to make sure I have RLock while reading LastTransaction.
Signed-off-by: Nadia Pinaeva <[email protected]>
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danwinship, npinaeva The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Before, calling
fake.Runandfake.Dumpfrom different goroutines caused a data race.