Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

markets: dcrdex candlesticks #1854

Merged
merged 6 commits into from Sep 1, 2021
Merged

markets: dcrdex candlesticks #1854

merged 6 commits into from Sep 1, 2021

Conversation

buck54321
Copy link
Member

No description provided.

Copy link
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice surprise. Works well and deployed with a couple tweaks to the rate server, explorer instances picking up the new feed: https://dcrdata.decred.org/market?chart=candlestick&xc=dcrdex&bin=1h

Mostly easy comments, but it does look like the dcr.reqs map is susceptible to a concurrent map read-write.

Also, when testing, I noticed a data race that was already on master:

WARNING: DATA RACE
Read at 0x00c000011750 by goroutine 104:
  github.com/decred/dcrdata/exchanges/v3.(*ExchangeBot).signalIndexUpdate()
      /home/jon/github/decred/dcrdata/exchanges/bot.go:575 +0x8a
  github.com/decred/dcrdata/exchanges/v3.(*ExchangeBot).Start()
      /home/jon/github/decred/dcrdata/exchanges/bot.go:483 +0xf50
  main._main·dwrap·3()
      /home/jon/github/decred/dcrdata/cmd/dcrdata/main.go:421 +0x64

Previous write at 0x00c000011750 by goroutine 136:
  github.com/decred/dcrdata/exchanges/v3.(*ExchangeBot).UpdateChannels()
      /home/jon/github/decred/dcrdata/exchanges/bot.go:554 +0x2a4
  github.com/decred/dcrdata/cmd/dcrdata/explorer.(*explorerUI).watchExchanges()
      /home/jon/github/decred/dcrdata/cmd/dcrdata/explorer/explorer.go:788 +0x8e
  github.com/decred/dcrdata/cmd/dcrdata/explorer.New·dwrap·2()
      /home/jon/github/decred/dcrdata/cmd/dcrdata/explorer/explorer.go:383 +0x39

Goroutine 104 (running) created at:
  main._main()
      /home/jon/github/decred/dcrdata/cmd/dcrdata/main.go:421 +0xa309
...

Goroutine 136 (running) created at:
  github.com/decred/dcrdata/cmd/dcrdata/explorer.New()
      /home/jon/github/decred/dcrdata/cmd/dcrdata/explorer/explorer.go:383 +0x212a
  main._main()
      /home/jon/github/decred/dcrdata/cmd/dcrdata/main.go:457 +0x47e8

I think it's straightforward to resolve:

diff --git a/exchanges/bot.go b/exchanges/bot.go
index 9f9c66ab..9b4621f7 100644
--- a/exchanges/bot.go
+++ b/exchanges/bot.go
@@ -216,15 +216,6 @@ type UpdateChannels struct {
        Quit     chan struct{}
 }
 
-// NewUpdateChannels creates a new initialized set of UpdateChannels.
-func NewUpdateChannels() *UpdateChannels {
-       return &UpdateChannels{
-               Exchange: make(chan *ExchangeUpdate, 16),
-               Index:    make(chan *IndexUpdate, 16),
-               Quit:     make(chan struct{}),
-       }
-}
-
 // The chart data structures that are encoded and cached are the
 // candlestickResponse and the depthResponse.
 type candlestickResponse struct {
@@ -562,6 +553,8 @@ func (bot *ExchangeBot) UpdateChannels() *UpdateChannels {
 
 // Send an update to any channels requested with bot.UpdateChannels().
 func (bot *ExchangeBot) signalExchangeUpdate(update *ExchangeUpdate) {
+       bot.mtx.RLock()
+       defer bot.mtx.RUnlock()
        for _, ch := range bot.updateChans {
                select {
                case ch <- update:
@@ -572,6 +565,8 @@ func (bot *ExchangeBot) signalExchangeUpdate(update *ExchangeUpdate) {
 }
 
 func (bot *ExchangeBot) signalIndexUpdate(update *IndexUpdate) {
+       bot.mtx.RLock()
+       defer bot.mtx.RUnlock()
        for _, ch := range bot.indexChans {
                select {
                case ch <- update:

NewUpdateChannels was unused, and despite being exported, I cannot imagine a use for it.

exchanges/exchanges.go Outdated Show resolved Hide resolved
exchanges/exchanges.go Show resolved Hide resolved
exchanges/exchanges.go Show resolved Hide resolved
exchanges/exchanges.go Outdated Show resolved Hide resolved
exchanges/exchanges.go Outdated Show resolved Hide resolved
exchanges/exchanges.go Outdated Show resolved Hide resolved
exchanges/exchanges.go Outdated Show resolved Hide resolved
exchanges/exchanges.go Outdated Show resolved Hide resolved
exchanges/exchanges.go Show resolved Hide resolved
exchanges/exchanges.go Outdated Show resolved Hide resolved
@chappjc chappjc added this to the 6.0 milestone Sep 1, 2021
@chappjc chappjc merged commit 76e33ee into decred:master Sep 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants