*: use go-deadlock in race builds

[tamird]

Fixes #7972.

---

This change is

[sasha-s]

Review status: 0 of 67 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

util/syncutil/mutex_deadlock.go, line 21 [r1] (raw file):

package syncutil

import deadlock "github.com/sasha-s/go-deadlock"

Deadlock is doing os.Exit(2) by default when it sees a potential deadlock.
If that's not what you need, you might want to change deadlock.Opts.OnPotentialDeadlock in init.

---

Comments from Reviewable

[tamird]

Review status: 0 of 67 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

util/syncutil/mutex_deadlock.go, line 21 [r1] (raw file):

Previously, sasha-s wrote…

Deadlock is doing os.Exit(2) by default when it sees a potential deadlock.
If that's not what you need, you might want to change deadlock.Opts.OnPotentialDeadlock in init.

Yeah, I saw. @mberhault any opinions on this? Will affect rho.

---

Comments from Reviewable

[bdarnell]

Can you put the substantive changes (e.g. gossip.go, and maybe others I haven't found) in a separate commit from the one that does s/sync.Mutex/syncutil.Mutex/g?

---

Review status: 0 of 67 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

---

Comments from Reviewable

[tamird]

@bdarnell done.

---

Review status: 0 of 67 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.

---

Comments from Reviewable

[bdarnell]

LGTM.

Since CI is failing in non-trivial ways and updating this PR is going to be painful, you may want to merge the renaming to use syncutil everywhere without making syncutil use go-deadlock.

---

Review status: 0 of 67 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

---

build/check-style.sh, line 39 [r6] (raw file):

TestSyncMutex() {
  echo "checking for sync.Mutex usage (use syncutil.Mutex instead)"
  ! git grep -nE 'sync\.Mutex' -- '*.go' | grep -vE '^util/syncutil/mutex_sync\.go\b'

sync\.(RW)?Mutex

---

util/syncutil/mutex_deadlock.go, line 21 [r1] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Yeah, I saw. @mberhault any opinions on this? Will affect rho.

I think ideally I'd like this to work like the race detector: log it (and in tests report the test as failed), but don't kill the process. But if that's a pain (and integrating it with the test reporting might be trouble), I'm ok with just killing the process (even on rho).

---

Comments from Reviewable

[tamird]

Review status: 0 of 67 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

build/check-style.sh, line 39 [r6] (raw file):

Previously, bdarnell (Ben Darnell) wrote…

sync\.(RW)?Mutex

Done.

---

util/syncutil/mutex_deadlock.go, line 21 [r1] (raw file):

Previously, bdarnell (Ben Darnell) wrote…

I think ideally I'd like this to work like the race detector: log it (and in tests report the test as failed), but don't kill the process. But if that's a pain (and integrating it with the test reporting might be trouble), I'm ok with just killing the process (even on rho).

We could piggyback on leaktest in order to get a handle to the `testing.T`, perhaps?

---

Comments from Reviewable

[mberhault]

Review status: 0 of 67 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

util/syncutil/mutex_deadlock.go, line 21 [r1] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

We could piggyback on leaktest in order to get a handle to the testing.T, perhaps?

I prefer having the option of dying of not (the race detector has that option), but as long as we don't get false positives, I don't mind exits too much.

---

Comments from Reviewable

[bdarnell]

Review status: 0 of 67 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

util/syncutil/mutex_deadlock.go, line 21 [r1] (raw file):

Previously, mberhault (marc) wrote…

I prefer having the option of dying of not (the race detector has that option), but as long as we don't get false positives, I don't mind exits too much.

You mean have `leaktest` put the `testing.T` in a public global? Eww. What I had in mind with integrating it with test reporting was to parse it out of the logs like go2xunit does for races: https://bitbucket.org/tebeka/go2xunit/src/bca3a606b7abccfdfcab96b2a532becba46dceeb/parsers.go?fileviewer=file-view-default#parsers.go-47

---

Comments from Reviewable

[tamird]

Review status: 0 of 67 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

util/syncutil/mutex_deadlock.go, line 21 [r1] (raw file):

Previously, bdarnell (Ben Darnell) wrote…

You mean have leaktest put the testing.T in a public global? Eww. What I had in mind with integrating it with test reporting was to parse it out of the logs like go2xunit does for races: https://bitbucket.org/tebeka/go2xunit/src/bca3a606b7abccfdfcab96b2a532becba46dceeb/parsers.go?fileviewer=file-view-default#parsers.go-47

Ew indeed.

Well, I guess I'll leave it as-is for now, especially since we're not going to use go-deadlock for now.

---

Comments from Reviewable