-
Notifications
You must be signed in to change notification settings - Fork 5
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
Refactor #28
Refactor #28
Changes from 1 commit
ee0c8b6
a0df53e
ff34dee
fa42464
ad698d1
7f587a0
2538e07
7c231f3
c6518d2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,8 @@ package krakenfeeder | |
import ( | ||
"encoding/json" | ||
"fmt" | ||
"sync" | ||
"time" | ||
|
||
"github.com/gorilla/websocket" | ||
"github.com/shopspring/decimal" | ||
|
@@ -17,23 +19,33 @@ const ( | |
) | ||
|
||
type service struct { | ||
conn *websocket.Conn | ||
conn *websocket.Conn | ||
writeTicker *time.Ticker | ||
lock *sync.RWMutex | ||
chLock *sync.Mutex | ||
|
||
marketByTicker map[string]ports.Market | ||
lastPriceFeed ports.PriceFeed | ||
feedChan chan ports.PriceFeed | ||
quitChan chan struct{} | ||
} | ||
|
||
func NewKrakenPriceFeeder(args ...interface{}) (ports.PriceFeeder, error) { | ||
if len(args) != 1 { | ||
if len(args) != 2 { | ||
return nil, fmt.Errorf("invalid number of args") | ||
} | ||
|
||
markets, ok := args[0].([]ports.Market) | ||
interval, ok := args[0].(int) | ||
if !ok { | ||
return nil, fmt.Errorf("unknown args type") | ||
return nil, fmt.Errorf("unknown interval arg type") | ||
} | ||
|
||
markets, ok := args[1].([]ports.Market) | ||
if !ok { | ||
return nil, fmt.Errorf("unknown marktes arg type") | ||
} | ||
|
||
writeTicker := time.NewTicker(time.Duration(interval) * time.Millisecond) | ||
mktTickers := make([]string, 0, len(markets)) | ||
mktByTicker := make(map[string]ports.Market) | ||
for _, mkt := range markets { | ||
|
@@ -49,24 +61,22 @@ func NewKrakenPriceFeeder(args ...interface{}) (ports.PriceFeeder, error) { | |
|
||
return &service{ | ||
conn: conn, | ||
writeTicker: writeTicker, | ||
lock: &sync.RWMutex{}, | ||
chLock: &sync.Mutex{}, | ||
marketByTicker: mktByTicker, | ||
feedChan: make(chan ports.PriceFeed), | ||
quitChan: make(chan struct{}, 1), | ||
}, nil | ||
} | ||
|
||
func (k *service) Start() error { | ||
defer func() { | ||
close(k.feedChan) | ||
close(k.quitChan) | ||
}() | ||
|
||
mustReconnect, err := k.start() | ||
func (s *service) Start() error { | ||
mustReconnect, err := s.start() | ||
for mustReconnect { | ||
log.WithError(err).Warn("connection dropped unexpectedly. Trying to reconnect...") | ||
|
||
tickers := make([]string, 0, len(k.marketByTicker)) | ||
for ticker := range k.marketByTicker { | ||
tickers := make([]string, 0, len(s.marketByTicker)) | ||
for ticker := range s.marketByTicker { | ||
tickers = append(tickers, ticker) | ||
} | ||
|
||
|
@@ -75,54 +85,92 @@ func (k *service) Start() error { | |
if err != nil { | ||
return err | ||
} | ||
k.conn = conn | ||
s.conn = conn | ||
|
||
log.Debug("connection and subscriptions re-established. Restarting...") | ||
mustReconnect, err = k.start() | ||
mustReconnect, err = s.start() | ||
} | ||
|
||
return err | ||
} | ||
|
||
func (k *service) Stop() { | ||
k.quitChan <- struct{}{} | ||
func (s *service) Stop() { | ||
s.quitChan <- struct{}{} | ||
} | ||
|
||
func (k *service) FeedChan() chan ports.PriceFeed { | ||
return k.feedChan | ||
func (s *service) FeedChan() chan ports.PriceFeed { | ||
return s.feedChan | ||
} | ||
|
||
func (k *service) start() (mustReconnect bool, err error) { | ||
func (s *service) start() (mustReconnect bool, err error) { | ||
defer func() { | ||
if rec := recover(); rec != nil { | ||
mustReconnect = true | ||
} | ||
}() | ||
|
||
go func() { | ||
for range s.writeTicker.C { | ||
s.writeToFeedChan() | ||
} | ||
}() | ||
|
||
for { | ||
select { | ||
case <-k.quitChan: | ||
err = k.conn.Close() | ||
case <-s.quitChan: | ||
s.writeTicker.Stop() | ||
s.closeChannels() | ||
err = s.conn.Close() | ||
return false, err | ||
default: | ||
_, message, err := k.conn.ReadMessage() | ||
_, message, err := s.conn.ReadMessage() | ||
if err != nil { | ||
if websocket.IsUnexpectedCloseError(err, websocket.CloseGoingAway, websocket.CloseAbnormalClosure) { | ||
panic(err) | ||
} | ||
} | ||
|
||
priceFeed := k.parseFeed(message) | ||
priceFeed := s.parseFeed(message) | ||
if priceFeed == nil { | ||
continue | ||
} | ||
|
||
k.feedChan <- priceFeed | ||
s.writePriceFeed(priceFeed) | ||
} | ||
} | ||
} | ||
|
||
func (k *service) parseFeed(msg []byte) ports.PriceFeed { | ||
func (s *service) readPriceFeed() ports.PriceFeed { | ||
s.lock.RLock() | ||
defer s.lock.RUnlock() | ||
return s.lastPriceFeed | ||
} | ||
|
||
func (s *service) writePriceFeed(priceFeed ports.PriceFeed) { | ||
s.lock.Lock() | ||
defer s.lock.Unlock() | ||
s.lastPriceFeed = priceFeed | ||
} | ||
|
||
func (s *service) writeToFeedChan() { | ||
s.chLock.Lock() | ||
defer s.chLock.Unlock() | ||
|
||
priceFeed := s.readPriceFeed() | ||
if priceFeed != nil { | ||
s.feedChan <- priceFeed | ||
} | ||
} | ||
|
||
func (s *service) closeChannels() { | ||
s.chLock.Lock() | ||
defer s.chLock.Unlock() | ||
|
||
close(s.feedChan) | ||
close(s.quitChan) | ||
} | ||
|
||
func (s *service) parseFeed(msg []byte) ports.PriceFeed { | ||
var i []interface{} | ||
if err := json.Unmarshal(msg, &i); err != nil { | ||
return nil | ||
|
@@ -136,7 +184,7 @@ func (k *service) parseFeed(msg []byte) ports.PriceFeed { | |
return nil | ||
} | ||
|
||
mkt, ok := k.marketByTicker[ticker] | ||
mkt, ok := s.marketByTicker[ticker] | ||
if !ok { | ||
return nil | ||
} | ||
|
@@ -168,7 +216,7 @@ func (k *service) parseFeed(msg []byte) ports.PriceFeed { | |
return &priceFeed{ | ||
market: mkt, | ||
price: &price{ | ||
basePrice: basePrice.String(), | ||
basePrice: basePrice.StringFixed(8), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is it possible to have a float with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. only 3 decimals seems not enough to me. For BTC/USD ,for example, the base price (hence 1/(BTCUSD price) expresses a BTC unit, therefore 8 decimal precision. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 8 decimals plus the dot and the base unit, therefore |
||
quotePrice: quotePrice.String(), | ||
}, | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not great to panic here deep in code, better to lift up eventually and handle in
cmd
maybe?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this the fix for #24 ? shoudnt we connect by ourself again via code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is specifically intended. After debugging and testing how this issue affects our service, basically what happens when Cloudflare closes the connection is that k.conn.ReadMessage() panics instead of returning the IsUnexpectedCloseError.
For this reason, we MUST recover the paniced code from the websocket pkg and signal that a reconnection attempt must be performed. In case the error is detected by this line (but I never experienced this so far) , we panic like websocket would be to preserve the behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe better to add some commented line to explain why things are done this way, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes then comment