client/dex: BookUpdate notification races

[norwnd]

Resolves #2188. Addresses the rest of the things discussed.

[norwnd]

It seems after #1208 this feed must be called with the bookie.mtx locked part is no longer correct/relevant ?

I changed the comment phrasing to reflect latest renames but since bookie.feedsMtx is locked below and I don't see it being locked before outer func calls newFeed suggests to me 1) comment is no longer relevant 2) locking itself is fine; So we should probably erase this comment part completely now, right ?

[chappjc]

We're committing the cardinal sin of closing the channels in a goroutine that is not the sender.
I think the comment may be alluding to the added duties of the mutex there, but not sure. I feel like the comment may have missed the with "calling" when it's meant "accessed" (referring to channel as feed)

[norwnd]

Updated comments according to my current understanding (based on relevant commit here d11f1ce).

[norwnd]

Ok, I think I've figured it out, added more comments on what I think is going on there: 33fd0a0, not quite in that commit - just see the latest PR diff instead.

Just as an example, handleBookOrderMsg is one place where we can drop order book update meant to be relayed to bookie.feeds if we release dc.booksMtx before the feed was properly created (and relayed initial "book" message) with bookie.newFeed.

And If I'm not missing something we can probably just prepare bookie locally first and only then grab dc.booksMtx to insert it into dc.books map ? I'd want to look into that some more.

note, we only ever use 1 feed per bookie right now

Ah well, I got that ^ part wrong; so we need to lock dc.booksMtx to prevent initialising same bookie twice when in fact what we want is to have 1 bookie and 2 feeds in such case.

[norwnd]

I'm digging a bit deeper though, more stuff is coming out. Squashed existing commits and put PR in Draft for now.

[buck54321]

Once again, a norwnd PR that does nothing. You're effectively only undoing the shit I already told you not to do in your last PR. You pull this shit again, and I'll revoke your contractor request. Stop wasting our time.

[norwnd]

Well, it does fix these races for me:

WARNING: DATA RACE
Read at 0x00c000ed2e00 by goroutine 41:
  decred.org/dcrdex/client/orderbook.(*OrderBook).book()
      /Users/norwnd/dcrdex/client/orderbook/orderbook.go:311 +0x64
  decred.org/dcrdex/client/orderbook.(*OrderBook).Book()
      /Users/norwnd/dcrdex/client/orderbook/orderbook.go:361 +0x1b1
  decred.org/dcrdex/client/core.handleBookOrderMsg()
      /Users/norwnd/dcrdex/client/core/bookie.go:659 +0x196
  decred.org/dcrdex/client/core.(*Core).listen.func1()
      /Users/norwnd/dcrdex/client/core/core.go:8260 +0x156
  decred.org/dcrdex/client/core.(*Core).listen.func2()
      /Users/norwnd/dcrdex/client/core/core.go:8274 +0xcc

Previous write at 0x00c000ed2e00 by goroutine 306:
  decred.org/dcrdex/client/orderbook.(*OrderBook).Reset()
      /Users/norwnd/dcrdex/client/orderbook/orderbook.go:249 +0x11a
  decred.org/dcrdex/client/core.(*Core).handleReconnect.func2()
      /Users/norwnd/dcrdex/client/core/core.go:7924 +0x27e
  decred.org/dcrdex/client/core.(*Core).handleReconnect()
      /Users/norwnd/dcrdex/client/core/core.go:7951 +0x171e
  decred.org/dcrdex/client/core.(*Core).connectDEXWithFlag.func2.1()
      /Users/norwnd/dcrdex/client/core/core.go:7691 +0x58

and this one too

WARNING: DATA RACE
Write at 0x00c0006701d0 by goroutine 1359:
  decred.org/dcrdex/client/orderbook.(*OrderBook).Reset()
      /Users/norwnd/dcrdex/client/orderbook/orderbook.go:249 +0x11a
  decred.org/dcrdex/client/core.(*Core).handleReconnect.func2()
      /Users/norwnd/dcrdex/client/core/core.go:7924 +0x27e
  decred.org/dcrdex/client/core.(*Core).handleReconnect()
      /Users/norwnd/dcrdex/client/core/core.go:7951 +0x171e
  decred.org/dcrdex/client/core.(*Core).connectDEXWithFlag.func2.1()
      /Users/norwnd/dcrdex/client/core/core.go:7691 +0x58

Previous write at 0x00c0006701d0 by goroutine 1341:
  decred.org/dcrdex/client/orderbook.(*OrderBook).Reset()
      /Users/norwnd/dcrdex/client/orderbook/orderbook.go:249 +0x11a
  decred.org/dcrdex/client/core.(*Core).handleReconnect.func2()
      /Users/norwnd/dcrdex/client/core/core.go:7924 +0x27e
  decred.org/dcrdex/client/core.(*Core).handleReconnect()
      /Users/norwnd/dcrdex/client/core/core.go:7951 +0x171e
  decred.org/dcrdex/client/core.(*Core).connectDEXWithFlag.func2.1()
      /Users/norwnd/dcrdex/client/core/core.go:7691 +0x58

basically proving my statement in comments:

                 // Locking on dc.booksMtx is currently the only way to ensure the "book"
		// notification gets sent first, before we start applying any update
		// notifications coming from server (that can potentially happen once
		// booky.Reset func returns).