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

Implement incremental subscriptions on the client #2111

Merged
merged 23 commits into from
Jan 31, 2025
Merged

Conversation

jsdt
Copy link
Contributor

@jsdt jsdt commented Jan 13, 2025

Description of Changes

This implements subscriptions to individual queries in the rust SDK.

Most of the logic is in SubscriptionState, which is the source of truth for the state of a subscription. Most of the complexity comes from the use of the PendingMutation queue to delay operations, since some things can change by the time we call that callback.

There are some important followup changes needed:

  1. This will not handle queries with overlapping results well (I think it will panic). This shouldn't be a regression from how that would happen with the current API, but it's worth calling out. We need to add ref counting of rows to handle that correctly.
  2. Handling of errors for specific queries has a test, but handling a fatal error is not. I plan to implement that cleanup logic in a separate PR.

This also includes a server-side change to actually send SubscriptionError messages.

The corresponding private PR is https://github.com/clockworklabs/SpacetimeDBPrivate/pull/1279.

API and ABI breaking changes

If this is an API or ABI breaking change, please apply the
corresponding GitHub label.

Expected complexity level and risk

  1. This is reasonably complicated, since some of the different concurrency cases will be difficult to test.

Testing

There are some basic tests in sdk/tests/test-client/src/main.rs that cover subscribing and unsubscribing to a query, and handling an error from subscribing to an invalid query.

@gefjon
Copy link
Contributor

gefjon commented Jan 17, 2025

From out-of-band discussion: what happens if a client calls unsubscribe_then multiple times on (handles for) the same query? My intuition is all but the first call panic, but I'm not married to that. Perhaps related is, what happens if a client calls unsubscribe_then on a handle that has already errored? My intuition is that it's a no-op, but I find that at odds with the first thing.

@jsdt
Copy link
Contributor Author

jsdt commented Jan 17, 2025

From out-of-band discussion: what happens if a client calls unsubscribe_then multiple times on (handles for) the same query? My intuition is all but the first call panic, but I'm not married to that. Perhaps related is, what happens if a client calls unsubscribe_then on a handle that has already errored? My intuition is that it's a no-op, but I find that at odds with the first thing.

In both cases, unsubscribe_then will just return an error. IMO panicking for something like that would be not be very user friendly.

@gefjon
Copy link
Contributor

gefjon commented Jan 17, 2025

In both cases, unsubscribe_then will just return an error. IMO panicking for something like that would be not be very user friendly.

I had forgotten that it returned a Result. Returning Err is clearly correct, given that. Hooray!

@kazimuth
Copy link
Contributor

#2163 will also need to be merged in some form if we want to get the C# server module version of Blackholio working. Currently it's based at this branch but if we want to merge this first I can rebase it to master.

kazimuth added a commit to clockworklabs/com.clockworklabs.spacetimedbsdk that referenced this pull request Jan 23, 2025
## Description of Changes

As proposed. No upstream codegen changes needed :)

## API

 - [x] This is an API breaking change to the SDK

*If the API is breaking, please state below what will break*
The subscription API is slightly different.

## Requires SpacetimeDB PRs
clockworklabs/SpacetimeDB#2111

## Testsuite

SpacetimeDB branch name: jsdt/subscribe-sdk-3

## Testing
So far I have performed manual testing with the chat example. Working on
updating the unity and unit tests.

- [ ] Describe a test for this PR that you have completed
@jsdt jsdt enabled auto-merge January 31, 2025 18:00
@jsdt jsdt added this pull request to the merge queue Jan 31, 2025
Merged via the queue into master with commit e2ffc07 Jan 31, 2025
13 of 14 checks passed
@bfops bfops added the breaks library compatibility This change breaks the SpacetimeDB library interface label Jan 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backward-compatible breaks library compatibility This change breaks the SpacetimeDB library interface release-1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants