From 36a4bf4c1fcc7fa0f74b847c84bac92d0edcec1d Mon Sep 17 00:00:00 2001 From: Scott Date: Thu, 29 Apr 2021 11:27:19 +1000 Subject: [PATCH] Bugfix: Orderbook locks (#666) --- engine/routines_test.go | 10 ------- exchanges/binance/binance_websocket.go | 1 + exchanges/orderbook/calculator_test.go | 21 +++++++++++--- exchanges/orderbook/node_test.go | 14 +++++----- exchanges/orderbook/orderbook.go | 2 +- exchanges/orderbook/orderbook_test.go | 38 ++++++++++++++------------ 6 files changed, 46 insertions(+), 40 deletions(-) diff --git a/engine/routines_test.go b/engine/routines_test.go index af49f40a..7f7a4284 100644 --- a/engine/routines_test.go +++ b/engine/routines_test.go @@ -81,16 +81,6 @@ func TestHandleData(t *testing.T) { t.Error("Expected order to be modified to Active") } - err = b.WebsocketDataHandler(exchName, &order.Cancel{ - Exchange: fakePassExchange, - ID: orderID, - }) - if err != nil { - t.Error(err) - } - if origOrder.Status != order.Cancelled { - t.Error("Expected order status to be cancelled") - } // Send some gibberish err = b.WebsocketDataHandler(exchName, order.Stop) if err != nil { diff --git a/exchanges/binance/binance_websocket.go b/exchanges/binance/binance_websocket.go index 7f6b4f5c..b277447f 100644 --- a/exchanges/binance/binance_websocket.go +++ b/exchanges/binance/binance_websocket.go @@ -544,6 +544,7 @@ func (b *Binance) Subscribe(channelsToSubscribe []stream.ChannelSubscription) er for i := range channelsToSubscribe { payload.Params = append(payload.Params, channelsToSubscribe[i].Channel) } + err := b.Websocket.Conn.SendJSONMessage(payload) if err != nil { return err diff --git a/exchanges/orderbook/calculator_test.go b/exchanges/orderbook/calculator_test.go index 69fea175..ef5d4b88 100644 --- a/exchanges/orderbook/calculator_test.go +++ b/exchanges/orderbook/calculator_test.go @@ -1,6 +1,7 @@ package orderbook import ( + "errors" "testing" "github.com/thrasher-corp/gocryptotrader/currency" @@ -32,14 +33,26 @@ func TestWhaleBomb(t *testing.T) { } // valid - b.WhaleBomb(7001, true) + _, err = b.WhaleBomb(7001, true) + if !errors.Is(err, nil) { + t.Errorf("received '%v', expected '%v'", err, nil) + } // invalid - b.WhaleBomb(7002, true) + _, err = b.WhaleBomb(7002, true) + if err == nil { + t.Error("unexpected result") + } // valid - b.WhaleBomb(6998, false) + _, err = b.WhaleBomb(6998, false) + if !errors.Is(err, nil) { + t.Errorf("received '%v', expected '%v'", err, nil) + } // invalid - b.WhaleBomb(6997, false) + _, err = b.WhaleBomb(6997, false) + if err == nil { + t.Error("unexpected result") + } } func TestSimulateOrder(t *testing.T) { diff --git a/exchanges/orderbook/node_test.go b/exchanges/orderbook/node_test.go index 553a675c..faebd86f 100644 --- a/exchanges/orderbook/node_test.go +++ b/exchanges/orderbook/node_test.go @@ -9,9 +9,9 @@ import ( func TestPushPop(t *testing.T) { s := newStack() - var nSlice []*node + var nSlice [100]*node for i := 0; i < 100; i++ { - nSlice = append(nSlice, s.Pop()) + nSlice[i] = s.Pop() } if s.getCount() != 0 { @@ -29,9 +29,9 @@ func TestPushPop(t *testing.T) { func TestCleaner(t *testing.T) { s := newStack() - var nSlice []*node + var nSlice [100]*node for i := 0; i < 100; i++ { - nSlice = append(nSlice, s.Pop()) + nSlice[i] = s.Pop() } tn := getNow() @@ -39,16 +39,16 @@ func TestCleaner(t *testing.T) { s.Push(nSlice[i], tn) } // Makes all the 50 pushed nodes invalid - time.Sleep(time.Millisecond * 550) + time.Sleep(time.Millisecond * 260) tn = getNow() for i := 50; i < 100; i++ { s.Push(nSlice[i], tn) } - time.Sleep(time.Millisecond * 550) + time.Sleep(time.Millisecond * 50) if s.getCount() != 50 { t.Fatalf("incorrect stack count expected %v but received %v", 50, s.getCount()) } - time.Sleep(time.Second) + time.Sleep(time.Millisecond * 350) if s.getCount() != 0 { t.Fatalf("incorrect stack count expected %v but received %v", 0, s.getCount()) } diff --git a/exchanges/orderbook/orderbook.go b/exchanges/orderbook/orderbook.go index 4c242ac3..78663995 100644 --- a/exchanges/orderbook/orderbook.go +++ b/exchanges/orderbook/orderbook.go @@ -98,6 +98,7 @@ func (s *Service) DeployDepth(exchange string, p currency.Pair, a asset.Item) (* return nil, errAssetTypeNotSet } s.Lock() + defer s.Unlock() m1, ok := s.books[strings.ToLower(exchange)] if !ok { id, err := s.Mux.GetID() @@ -125,7 +126,6 @@ func (s *Service) DeployDepth(exchange string, p currency.Pair, a asset.Item) (* book = newDepth(m1.ID) m3[p.Quote.Item] = book } - s.Unlock() return book, nil } diff --git a/exchanges/orderbook/orderbook_test.go b/exchanges/orderbook/orderbook_test.go index 760f3d04..d0894811 100644 --- a/exchanges/orderbook/orderbook_test.go +++ b/exchanges/orderbook/orderbook_test.go @@ -17,8 +17,8 @@ import ( func TestMain(m *testing.M) { // Sets up lower values for test environment - defaultInterval = time.Second - defaultAllowance = time.Millisecond * 500 + defaultInterval = time.Millisecond * 250 + defaultAllowance = time.Millisecond * 100 err := dispatch.Start(1, dispatch.DefaultJobsLimit) if err != nil { log.Fatal(err) @@ -159,7 +159,7 @@ func TestCalculateTotalBids(t *testing.T) { } } -func TestCalculateTotaAsks(t *testing.T) { +func TestCalculateTotalAsks(t *testing.T) { t.Parallel() curr, err := currency.NewPairFromStrings("BTC", "USD") if err != nil { @@ -204,14 +204,14 @@ func TestGetOrderbook(t *testing.T) { } _, err = Get("nonexistent", c, asset.Spot) - if err == nil { - t.Fatal("TestGetOrderbook retrieved non-existent orderbook") + if !errors.Is(err, errCannotFindOrderbook) { + t.Fatalf("received '%v', expected '%v'", err, errCannotFindOrderbook) } c.Base = currency.NewCode("blah") _, err = Get("Exchange", c, asset.Spot) - if err == nil { - t.Fatal("TestGetOrderbook retrieved non-existent orderbook using invalid first currency") + if !errors.Is(err, errCannotFindOrderbook) { + t.Fatalf("received '%v', expected '%v', using invalid first currency", err, errCannotFindOrderbook) } newCurrency, err := currency.NewPairFromStrings("BTC", "AUD") @@ -219,8 +219,8 @@ func TestGetOrderbook(t *testing.T) { t.Fatal(err) } _, err = Get("Exchange", newCurrency, asset.Spot) - if err == nil { - t.Fatal("TestGetOrderbook retrieved non-existent orderbook using invalid second currency") + if !errors.Is(err, errCannotFindOrderbook) { + t.Fatalf("received '%v', expected '%v', using invalid second currency", err, errCannotFindOrderbook) } base.Pair = newCurrency @@ -273,7 +273,7 @@ func TestGetDepth(t *testing.T) { t.Fatalf("expecting %s error but received %v", errCannotFindOrderbook, err) } - newCurrency, err := currency.NewPairFromStrings("BTC", "AUD") + newCurrency, err := currency.NewPairFromStrings("BTC", "DOGE") if err != nil { t.Fatal(err) } @@ -484,10 +484,12 @@ func TestProcessOrderbook(t *testing.T) { var catastrophicFailure bool for i := 0; i < 500; i++ { + m.Lock() if catastrophicFailure { + m.Unlock() break } - + m.Unlock() wg.Add(1) go func() { newName := "Exchange" + strconv.FormatInt(rand.Int63(), 10) // nolint:gosec // no need to import crypo/rand for testing @@ -509,36 +511,36 @@ func TestProcessOrderbook(t *testing.T) { if err != nil { t.Error(err) catastrophicFailure = true + m.Unlock() + wg.Done() return } - testArray = append(testArray, quick{Name: newName, P: newPairs, Bids: bids, Asks: asks}) m.Unlock() wg.Done() }() } + wg.Wait() if catastrophicFailure { t.Fatal("Process() error", err) } - wg.Wait() - for _, test := range testArray { wg.Add(1) fatalErr := false - go func(test quick) { - result, err := Get(test.Name, test.P, asset.Spot) + go func(q quick) { + result, err := Get(q.Name, q.P, asset.Spot) if err != nil { fatalErr = true return } - if result.Asks[0] != test.Asks[0] { + if result.Asks[0] != q.Asks[0] { t.Error("TestProcessOrderbook failed bad values") } - if result.Bids[0] != test.Bids[0] { + if result.Bids[0] != q.Bids[0] { t.Error("TestProcessOrderbook failed bad values") }