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

Add logging and optional callback to buffer fills #297

Merged
merged 3 commits into from
Aug 9, 2024

Conversation

leki75
Copy link
Contributor

@leki75 leki75 commented Jul 19, 2024

Slow client related issues:

  • client.maintainConnection waits for the messageProcessor goroutine(s) to finish to reconnect: this delays the reconnect and more messages can be missed
  • whenever the client buffer c.in is full the connReader goroutine is blocked: the TCP stack of the operating system will buffer the messages and can cause disconnects if the application gets the pong message late, moreover the connReader goroutine will not exit if the context is cancelled while it is blocked on writing to c.in

Slow client related issues:
* `client.maintainConnection` waits for the `messageProcessor` goroutine(s) to
  finish to reconnect: this delays the reconnect and more messages will be missed
* whenever the client buffer `c.in` is full the `connReader` goroutine is blocked
marketdata/stream/client.go Outdated Show resolved Hide resolved
Copy link
Contributor

@alanvoss alanvoss left a comment

Choose a reason for hiding this comment

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

Other than my hope that someday we can become diligent about having tests for most code changes, and a couple of naming nits, this looks good to me.

marketdata/stream/client.go Outdated Show resolved Hide resolved
marketdata/stream/options.go Outdated Show resolved Hide resolved
@gnvk gnvk changed the title Fix slow client issues Add logging and optional callback to buffer fills Aug 9, 2024
Copy link
Collaborator

@gnvk gnvk left a comment

Choose a reason for hiding this comment

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

Thank you @leki75 !

@gnvk gnvk merged commit 1c54c5a into alpacahq:master Aug 9, 2024
1 check passed
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.

3 participants