Fix recursive RLock() mutex acquisition

[smira]

There were at least two code paths which might acquire .RLock() recursively:

1. Setup*() -> createResult()
2. Setup*() -> Networks().

On its own, it's not a problem, but if .Load() is called concurrently, it might do .Lock() in between recursive .RLock()s, which would lead to a deadlock.

See #125

Fix by introducing a contract on the way mutex is acquired.

Add a test facility to verify for such mistakes via build tags:

• go test -race or go test -tags deadlocks activates deadlock detection

[buroa]

Thanks for taking a look at this @smira ❤️

[smira]

Thanks for taking a look at this @smira ❤️

Thanks for finding the bug, fixing is easy :)

[MikeZappa87]

@mikebrow this was my original approach, I thought this was the correct way as I was concerned about a deadlock with the merged approach.

[cpuguy83]

Using an interface for sync.RWMutex seems easier to manage and test explicitly for than using build tags?

[cpuguy83]

Well my point is you can have an interface in the struct and set it to use the deadlock.RWMutex in a test.

[smira]

it's possible, but build tags allows to exclude deadlock.RWMutex completely from the build if it's not race-enabled or has an explicit deadlocks build tag.

plus interface indirection has a slight performance hit

this change is equivalent to what it was before if not using race/build tag

[mikebrow]

i'm good either way.. not worried about performance of interface indirection for network device setup/tear down :-) Fair to change it in a separated PR if desired.

[cpuguy83]

I guess the issue here is, who is running this test?
CI is not setup for it.

[smira]

CI is setup for it, it runs go test -race which is enough to activate deadlock library.

[cpuguy83]

Ah I was doing a find in the ci.yml I see its in the actual Makefile.

[mikebrow]

@mikebrow this was my original approach, I thought this was the correct way as I was concerned about a deadlock with the merged approach.
nod...

the new contract
// Mutex contract:
// - lock in public methods: write lock when mutating the state, read lock when reading the state.
// - never lock in private methods.
is consistent with the "or" path here #123 (comment)

Have not seen the failure path, exactly, yet, but this pattern seems fine and is easier to follow.

[mikebrow]

LGTM

[smira]

Have not seen the failure path, exactly, yet, but this pattern seems fine and is easier to follow.

It was in the failure path:

1. Setup*() -> createResult() (createResult and Setup* both acquire it, and createResult is private)
2. Setup*() -> Networks(). (Setup calls public Networks, and both acquire it)