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.

Sign up for GitHub

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

Jump to bottom

fix: synchronize access to lastPingTime in ticker struct #99

Merged
merged 4 commits into from
Aug 31, 2023
Merged

fix: synchronize access to lastPingTime in ticker struct #99

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
677b191
fix: Synchronize access to lastPingTime in Ticker struct
abhinandkakkadi Aug 15, 2023
ac1d3d8
fix: Synchronize access to lastPingTime in Ticker struct second pass
abhinandkakkadi Aug 15, 2023
56c8bcf
updated lasPingTime visibility, gitignored .vscode/settings.json
abhinandkakkadi Aug 23, 2023
6a38e48
update: updated as specified
abhinandkakkadi Aug 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .vscode/settings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"githubPullRequests.ignoredPullRequestBranches": ["master"]
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

}
18 changes: 16 additions & 2 deletions ticker/ticker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ type Ticker struct {
subscribedTokens map[uint32]Mode

cancel context.CancelFunc

lastPingTimeMutex sync.Mutex
Copy link
Member

Choose a reason for hiding this comment

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

Should we consider making this a RWMutex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

// callbacks represents callbacks available in ticker.
Expand Down Expand Up @@ -339,6 +341,12 @@ 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 {
Expand Down Expand Up @@ -370,6 +378,12 @@ func (t *Ticker) triggerNoReconnect(attempt int) {
}
}

func (t *Ticker) SetLastPingTime(time time.Time) {
abhinandkakkadi marked this conversation as resolved.
Show resolved Hide resolved
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)
Expand Down Expand Up @@ -401,7 +415,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.lastPingTime) > dataTimeoutInterval {
if time.Since(t.GetLastPingTime()) > dataTimeoutInterval {
// Close the current connection without waiting for close frame
if t.Conn != nil {
t.Conn.Close()
Expand Down Expand Up @@ -431,7 +445,7 @@ func (t *Ticker) readMessage(ctx context.Context, wg *sync.WaitGroup) {
}

// Update last ping time to check for connection
t.lastPingTime = time.Now()
t.SetLastPingTime(time.Now())

// Trigger message.
t.triggerMessage(mType, msg)
Expand Down