fix: synchronize access to lastPingTime in ticker struct

[abhinandkakkadi]

Modified the Ticker struct to include a mutex for synchronizing access to the lastPingTime property. This change ensures that the lastPingTime property is accessed safely by multiple goroutines concurrently.

• Added lastPingTimeMutex to Ticker struct
• Introduced SetLastPingTime and GetLastPingTime methods to safely set and get lastPingTime
• Updated readMessage function to use SetLastPingTime to update lastPingTime
• Updated checkConnection function to use GetLastPingTime to access lastPingTime

Resolves: Data race issue in Ticker methods

Closes #98

[rhnvrm]

Can we remove this file?

[abhinandkakkadi]

yes

[rhnvrm]

Should we consider making this a RWMutex?

[abhinandkakkadi]

I think Mutex would be a good choice in this situation. RWMutex is generally used where you want to concurrently reads and exclusive writes. here you have to perfrom read and write operations on the same variable, and you have to make sure that while one is performed other one is not done.

Using a Mutex you can be sure that only one thread can read or write to lastPingTime, preventing any concurrent access issues and ensuring consistencies

[abhinandkakkadi]

t.triggerError(fmt.Errorf("Error reading data: %v", err)) --

I have also noticed Some the way errors are formatted. It's a convention in go to not use Capital letters in error

[abhinandkakkadi]

• Updated the visibility of setLastPingTime and getLastPingTime
• removed .vscode/settings.json
• In this case Mutex is better than RWMutex

[rhnvrm]

diff --git a/ticker/ticker.go b/ticker/ticker.go
index 246a6eb..a10361c 100644
--- a/ticker/ticker.go
+++ b/ticker/ticker.go
@@ -9,6 +9,7 @@ import (
 	"math"
 	"net/url"
 	"sync"
+	"sync/atomic"
 	"time"
 
 	"github.com/gorilla/websocket"
@@ -28,7 +29,7 @@ type Ticker struct {
 
 	url                 url.URL
 	callbacks           callbacks
-	lastPingTime        time.Time
+	lastPingTime        atomicTime
 	autoReconnect       bool
 	reconnectMaxRetries int
 	reconnectMaxDelay   time.Duration
@@ -39,8 +40,22 @@ type Ticker struct {
 	subscribedTokens map[uint32]Mode
 
 	cancel context.CancelFunc
+}
+
+// atomicTime is wrapper over time.Time to safely access
+// an updating timestamp concurrently.
+type atomicTime struct {
+	v atomic.Value
+}
+
+// Get returns the current timestamp.
+func (b *atomicTime) Get() time.Time {
+	return b.v.Load().(time.Time)
+}
 
-	lastPingTimeMutex sync.Mutex
+// Set sets the current timestamp.
+func (b *atomicTime) Set(value time.Time) {
+	b.v.Store(value)
 }
 
 // callbacks represents callbacks available in ticker.
@@ -313,7 +328,7 @@ func (t *Ticker) ServeWithContext(ctx context.Context) {
 			t.reconnectAttempt = 0
 
 			// Set current time as last ping time
-			t.lastPingTime = time.Now()
+			t.lastPingTime.Set(time.Now())
 
 			// Set on close handler
 			t.Conn.SetCloseHandler(t.handleClose)
@@ -341,12 +356,6 @@ func (t *Ticker) handleClose(code int, reason string) error {
 	return nil
 }
 
-func (t *Ticker) getLastPingTime() time.Time {
-	t.lastPingTimeMutex.Lock()
-	defer t.lastPingTimeMutex.Unlock()
-	return t.lastPingTime
-}
-
 // Trigger callback methods
 func (t *Ticker) triggerError(err error) {
 	if t.callbacks.onError != nil {
@@ -378,12 +387,6 @@ func (t *Ticker) triggerNoReconnect(attempt int) {
 	}
 }
 
-func (t *Ticker) setLastPingTime(time time.Time) {
-	t.lastPingTimeMutex.Lock()
-	defer t.lastPingTimeMutex.Unlock()
-	t.lastPingTime = time
-}
-
 func (t *Ticker) triggerMessage(messageType int, message []byte) {
 	if t.callbacks.onMessage != nil {
 		t.callbacks.onMessage(messageType, message)
@@ -415,7 +418,7 @@ func (t *Ticker) checkConnection(ctx context.Context, wg *sync.WaitGroup) {
 
 			// If last ping time is greater then timeout interval then close the
 			// existing connection and reconnect
-			if time.Since(t.getLastPingTime()) > dataTimeoutInterval {
+			if time.Since(t.lastPingTime.Get()) > dataTimeoutInterval {
 				// Close the current connection without waiting for close frame
 				if t.Conn != nil {
 					t.Conn.Close()
@@ -445,7 +448,7 @@ func (t *Ticker) readMessage(ctx context.Context, wg *sync.WaitGroup) {
 			}
 
 			// Update last ping time to check for connection
-			t.setLastPingTime(time.Now())
+			t.lastPingTime.Set(time.Now())
 
 			// Trigger message.
 			t.triggerMessage(mType, msg)
@@ -771,4 +774,3 @@ func convertPrice(seg uint32, val float64) float64 {
 		return val / 100.0
 	}
 }
-

I think it might be better to do it like this.

• I observed that we missed replacing the setter in one place (as well as missed by me in the previous review). This is because the getter/setter were not on the field but on the ticker struct
• It might be better to just use atomic here.

[abhinandkakkadi]

I think so too. Go forward with that 👍

[rhnvrm]

Would you like to update the PR? @abhinandkakkadi

@vividvilla can you also review this once if it seems ok?

[abhinandkakkadi]

I have done the updates you specified and have given a PR.

[rhnvrm]

LGTM 👍

Will wait for @vividvilla to also just go through this once, and then I'll merge it.

[vividvilla]

LGTM