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

fix: swift watch query issue #128

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

DominicGBauer
Copy link
Contributor

Description

There is an issue in the Swift SDK where if a lot of updates are added in a small timeframe it results in a buffer overflow for the table update emitting function. When the buffer overflow happens the emit is suspended for an indeterminant amount of time and because of this the watch query does not receive a re-query event timeously. It therefore does not re-run when one would expect resulting in it appearing that the watch query does not work.

This PR fixes this issue by:

  1. Increasing the buffer overflow amount

  2. Implements tryEmit as opposed to emit so that if the emit fails it instead results in a warning log as opposed to suspending. The reason this is preferred is that it gives a clear indication of the issue (if it ever comes up again) as opposed to leaving it to maybe suspended resulting in a hard-to-diagnose issue. We could also opt instead of using SUSPEND for the buffer overflow behaviour to rather use DROP_OLDEST or DROP_LATEST, however these are not ideal as we may lose a table update that a user expects resulting in a watch query not updating when it should.

  3. It will also only clear pending updates on a successful emission as opposed to previously where it always cleared pending updates even if the emit function was not run.

@rkistner
Copy link
Contributor

I'm haven't looked at the details here since I'm not that familiar with Kotlin, so apologies if what I'm saying here is either already done or not relevant, but just want to give my input on how we handle this in other SDKs.

In most cases, you don't care about any intermediate updates. Since we only track the updates on a table level, getting multiple buffered updates for "the todos table changed" isn't useful. In fact it's actually harmful in some cases, since you could get multiple updates each triggering a new watched query evaluation, while nothing actually changed between them at that point since you're getting old buffered updates.

In that sense, it sounds like something like DROP_OLDEST could work. But I'd go even further by only keeping a single event buffered.

The only caveat here is that you need to make sure to not drop changed tables from the update. I.e. if you had a buffered update for todos and then get an update for lists, it should be "merged" into a single update, rather than completely removing todos from the update.

Dart implementation for reference.

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.

2 participants