Skip to content

Conversation

@molon
Copy link

@molon molon commented May 5, 2025

  • All rest methods now provide proper context.Context support
  • Clarified behavior of context parameters in streaming methods:
    • In Connect method, ctx only controls the initial connection process, not the full stream lifecycle
    • To terminate a stream created by Connect, use the returned terminate function or the Terminate method
    • In StreamTradeUpdatesInBackground, ctx only controls the initial connection, stream continues running independently afterward
    • To stop a stream started by StreamTradeUpdatesInBackground, use the returned terminate function
  • Replaced time.Sleep with ctxtime.Sleep to ensure timely cancellation on context expiration
  • Removed default timeout from handleSubChange, now uniformly controlled by ctx
  • Added buffer to subChangeRequest.result to prevent deadlocks caused by handleSubChange error returns in edge cases

These changes improve context handling throughout the API, making resource management more predictable and preventing potential deadlocks.

molon added 14 commits February 2, 2024 21:11
- All `rest` methods provide `ctx` support
- The `ctx` of the `Connect` method is only used as `initialCtx`. If you want to cancel the connection, please use the `Terminate` method
- Change `time.Sleep` to `ctxtime.Sleep` to ensure timely cancellation
- Remove the default timeout of `handleSubChange`, uniformly controlled by `ctx`, and give `subChangeRequest.result` a `buffer` to prevent deadlock caused by `handleSubChange` error return in extreme cases.
…oncurrency support

- Updated bull-spread example to pass context in API calls.
- Modified internal sleep function to handle nil context gracefully.
- Refactored marketdata functions to include context for better request management.
- Ensured all relevant functions in marketdata package now accept context as a parameter.
- Moved ctxtime import to maintain consistency across files.
- Updated error wrapping in maintainConnection function for better error context.
…g and logging

- Updated timeout durations in TestSubscriptionTimeout for better clarity.
- Replaced log statements with log.Println for consistency.
- Refactored context handling in TestSubscriptionTwiceAcrossConnectionIssues.
- Ensured proper cancellation of contexts in relevant test cases.
…r better resource management

- Updated Connect methods in various clients to return a termination function alongside error handling.
- Enhanced StreamTradeUpdatesInBackground to provide a terminate function for stopping the stream.
- Improved context handling in examples to ensure proper termination of connections.
@molon molon requested review from gnvk and said-saifi as code owners May 5, 2025 19:51
@gnvk gnvk added the v4 Backward incompatible change that can added in the next major version label May 6, 2025
@gnvk
Copy link
Collaborator

gnvk commented May 6, 2025

@molon This is a giant backward incompatible change. There are two ways to get this merged:

  1. Once reviewed and approved merge it to the v4 branch. There are a couple more issues labeled with v4. Once we do them all, we can consider releasing a new major version of the SDK. One of the benefits of this approach is that we would encourage our users to migrate to the context-aware methods.
  2. Instead of adding context to all current client methods, we could duplicate them all. E.g. client.GetAccount() to client.GetAccountCtx(context.Context). This way we could keep the major version, however it's an even bigger change. Moreover, it will possibly increase maintenance cost in the future.

@molon
Copy link
Author

molon commented May 13, 2025

@gnvk

Sorry for the late reply. I prefer the first way.

Additionally, I'm debating one issue.
Since I changed the behavior of the ctx parameter in streaming methods, even if we go with the first way, should we rename streaming methods to make users more aware of this backward incompatibility?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v4 Backward incompatible change that can added in the next major version

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants