CyCraft / magnetar

A framework-agnostic syncing solution that auto-connects any DB/API with your local data store and has optimistic-UI built in 🌟
https://magnetar.cycraft.co
MIT License
43 stars 5 forks source link

Force a flush of batched updates to Firestore? #55

Closed wifisher closed 2 years ago

wifisher commented 3 years ago

Is there a method to force all of the batched updates out to Firestore?

In my use case, the user will slowly be updating the data of a document that, for the most part, only needs to be pushed out to Firestore periodically. I found syncDebounceMs that appears to let me delay the batch sync for an arbitrary amount of time. I could set this to a suitably large value (likely measured at a minute or more) to be sure that several updates get merged together while still slowly syncing to Firestore.

However, there are a few cases where I need to be certain that the data has been pushed out to Firestore. For example, when an operation will be triggered on the data by cloud functions, etc.

I know when this will be done, so it would be easy to make a call to a sync function. I could then wait for it to complete before triggering the remote operation.

This would allow me to keep the number of writes to Firestore down while the user is active while still achieving consistency when required.

Thanks.

mesqueeb commented 3 years ago

Good issue !! There are ways but they're undocumented and I gotta double check if it's clear which collection/doc you can flush. Will update later this week with info on this !!

mesqueeb commented 2 years ago

@wifisher I just realise if you keep it at a minute or more, remember that it's potentially dangerous in this case:

example document: todos/todo1

  1. user has stream open for todos/todo1
  2. makes makes a change to todos/todo1 on one device, the countdown timer starts from 1 minute
  3. the user opens the same app on another device
  4. on the other device the todos/todo1 looks outdated because the timer is still going
  5. on this other device the user makes a different change to todos/todo1
  6. now if your changes cause "conflict" and you have a condition where device 2 syncs first, then...
  7. device 1 will get the updates of device 2 through the stream, update them locally (it will look on device 1 like your changes were reverted, device 1 now shows the state of device 2)
  8. now the timer of 1 minute on device 1 finishes:
  9. the queue of changes will still go out to the server
  10. The end result of the doc in the database is the state of device 1, not 2, because the conflicting update of device 1 came in last
  11. device 1 only sees the changes of device 2 until the stream kicks in again now it receives the changes from device 1
  12. then device 1 will revert back to the latest state through the stream and get reverted back to the original state from that original countdown timer

it's really case specific, so probably a non-issue but I just thought I'd let you know.

mesqueeb commented 2 years ago

@wifisher you can "flush" a certain document you have pending changes for like so:

magnetar.collection('todos').doc('todo1').merge({ something: 123 }, { syncDebounceMs: 0 })

if you execute a write once with the added configuration of { syncDebounceMs: 0 } it will instantly sync that to the server alongside any other changes that were queued.

Is this enough in your case?

I can consider adding some sort of

magnetar.collection('todos').doc('todo1').flush()
// or
magnetar.collection('todos').doc('todo1').sync()

but I don't want to expand too much on the magnetar syntax because it's shared with all plugins.

I can also consider something like:

import { flush } from '@magnetarjs/plugin-firestore'

flush(magnetar.collection('todos').doc('todo1'))
// or
sync(magnetar.collection('todos').doc('todo1'))

Do you have any ideas for syntax and/or opinions on this?

wifisher commented 2 years ago

Thanks for the heads up on the consistency issue in your todo example. I have thought through multi-device sync issues and I think that they are manageable in my use-cases.

The primary use-case is simply a cloud backup of the data so if something happens to the device or when the user switches to a new device, all of the data is still available.

The next most likely use-case is the user starts a project on their device and then finishes it later on their computer when they are back in the office.

So, for the most part, I expect that the app will be pushing data out to Firestore without streaming enabled.

For the more rare use-cases where multiple devices are uses simultaneously, I plan to be able to signal that two or more apps are accessing the same project and turn on streaming with a short debounce for the associated project. That should keep all of the app instances in sync.

Although, I do expect that I will implement the streaming, short debounce implementation first since it works for all use-cases. The long debounce push only to Firestore will be a somewhat later optimization.

wifisher commented 2 years ago

With regards to the flush syntax, my preference would be this one:

magnetar.collection('todos').doc('todo1').flush()

I may not have new changes to write out when I need to do the flush so I would likely have to do the following otherwise:

magnetar.collection('todos').doc('todo1').merge({}, { syncDebounceMs: 0 })

I'm not sure how Magnetar handles a merge of an empty object.

The nice thing about the .sync() method is that I can simply pass the doc reference around my code to make changes to the doc and then finally to flush it out. Your last option requires an import to do the flush.

And I personally prefer .sync() over .flush() since it more closely matches the syncDebounceMs naming.

mesqueeb commented 2 years ago

@wifisher regarding this:

I plan to be able to signal that two or more apps are accessing the same project and turn on streaming with a short debounce for the associated project.

I just implemented this feature natively in Magnetar core. You can check the recent commits on production. : )

mesqueeb commented 2 years ago

Regarding the next message, thanks for your thoughts! Will get back on this in the near future, but maybe next year. Please experiment using the workaround until then.


support or PRs always welcome, they help the maintenance of Magnetar : )

wifisher commented 2 years ago

I haven't had any luck getting a syncDebounceMs of 0 to flush out any pending changes.

Using a syncDebounceMs of 0 with no pending merges works as expected.

However, doing so with a previously pending merge appears to just reset the debounce timer which still results in a potentially lengthy delay.

For example, I create a Magnetar instance with a syncDebounceMs of 10000 specified for the Firestore plugin.

Then docRef.merge({ data: 'a' })

Wait 5s and then docRef.merge({ data: 'b' }, { syncDebounceMs: 0 })

Wait 6s more and then docRef.merge({ data: 'c' }, { syncDebounceMs: 0 })

If syncDebounceMs of 0 was to result in any pending changes being flushed out, then I would expect the first two merges to be applied together at about 5s from the first call to merge. The third merge should then occur at about 11s from the first merge call.

Instead, what I am seeing is all three merges being applied together at about 21s after the first call to merge. It would appear that the debounce timer is simply being reset to the syncDebounceMs value of the first merge instead of picking up the new debounce value.


As for a PR, I'm afraid that I am not familiar enough with Typescript to write code in it. I can work my way through reading it but writing in it is still a challenge.

For financial support, I would you consider one time payments instead of monthly sponsorship?

mesqueeb commented 2 years ago

@wifisher aha I remembered my own code incorrectly.

I will look into a manual flush during next week. Please hold on a few more days!

mesqueeb commented 2 years ago

@wifisher I have released the feature request in the latest versions.

The implementation ended up changing a little bit, because the pending writes that stack up are in fact not linked to any one specific module, but rather saved in a global cached of the Firestore plugin itself. That's why this ended up being the implementation necessary:

Screenshot 2021-12-14 at 10 52 23

you have to execute syncPendingWrites which is a method available when you instantiate the Firestore plugin. Just be careful to not instantiate the Firestore plugin twice.

As you can see in the example, you could re-export the syncPendingWrites function so you can import it into other places of your app.


usage:

import { syncPendingWrites } from 'my-magnetar-setup.js'

await syncPendingWrites() // returns `Promise<void>`

// now everything's flushed!

Please let me know if this solves your requirements. If it does, I would greatly appreciate a PR on the documentation. You could write something on syncDebounceMs and syncPendingWrites

see: https://github.com/CyCraft/magnetar/blob/production/packages/docs/src/pages/Docs/Plugins.md

wifisher commented 2 years ago

I haven't had a lot of time to look at this yet, but I think that there are still a few issues with this.

First, when you say to be careful not to instantiate the Firestore plugin twice, why is that? I've been reviewing the code and the BatchSync instance appears to be unique to each instance of the Firestore plugin.

In looking through the test code, the createMagnetarInstance() function appears to be instantiating a new plugin for each test. Will this be a problem for tests of the syncPendingWrites() method?

It also appears that the Promise returned from syncPendingWrites() does not always get resolved.

I think that this is due to the implementation of forceSyncEarly() in BatchSync. If there is no countdown currently in process (ie state.countdown === null as set by executeSync()), it will not resolve the Promise it returns. If there is no countdown, it should resolve the Promise immediately.

mesqueeb commented 2 years ago

Hi @wifisher see my response below:

First, when you say to be careful not to instantiate the Firestore plugin twice, why is that? I've been reviewing the code and the BatchSync instance appears to be unique to each instance of the Firestore plugin. In looking through the test code, the createMagnetarInstance() function appears to be instantiating a new plugin for each test. Will this be a problem for tests of the syncPendingWrites() method?

For automated tests we work with a separate instance of magnetar for every test. But in real life production code the intended usage of Magnetar is that there is just 1 magnetar instance, and therefore, it has just 1 Firestore plugin instance.

Do you have a use case where you need multiple magnetar instances?

It also appears that the Promise returned from syncPendingWrites() does not always get resolved. I think that this is due to the implementation of forceSyncEarly() in BatchSync. If there is no countdown currently in process (ie state.countdown === null as set by executeSync()), it will not resolve the Promise it returns. If there is no countdown, it should resolve the Promise immediately.

Good catch! I've published:

that should resolve this issue. (see commit) Can you please check again?

mesqueeb commented 2 years ago

@wifisher does the fix presented in this comment https://github.com/CyCraft/magnetar/issues/55#issuecomment-993082906 solve your FR about flushing?

If so please close this issue : )

--
Magnetar was made with love by Luca Ban.
You cannot sponsor every project, but next time you do, think of this one for its prolonged maintenance. 💜

wifisher commented 2 years ago

@mesqueeb it appears to work, but only if executed asynchronously from the code making the modifications.

That is, the following does not work:

merge(...)
syncPendingWrites()

But this does:

merge(....)
setTimeout(() => {
   syncPendingWrites()
}, 1)

In the first example, syncPendingWrites() tries to force finish the countdown timer before batchSync has created it. So syncPendingWrites() has no effect.

For the second example, syncPendingWrites() is called after batchSync has created the countdown timer. In this case, it has the desired effect.

It might make sense to implement the setTimeout in forceSyncEarly() of batchSync. This would allow syncPendingWrites() to work better in the general case with the 1ms delay hidden behind the promise.

What I am not sure of is the amount of delay required. It might sufficient to just queue up the countdown's forceFinish() to run asynchronously with little to no delay. Just giving the queued up batchSync asynchronous functions a chance to run might be all that it needs.

Or maybe it needs an actual delay to allow batchSync some time to do its work. In this case, it would mean that slower devices would need more delay to function as expected. Hopefully it is not this case, but I don't know the code well enough to judge.

mesqueeb commented 2 years ago

@wifisher I'm not sure if I ever attended to your last point here. Can you remind me if this was ever addressed or not? : O

If not, this might be a potential fix: https://github.com/CyCraft/magnetar/pull/80

Can you have a quick look at the changes of this PR?

you can test it in the most recent versions:

wifisher commented 2 years ago

@mesqueeb I had to set this aside to work on a different project for a while but should be freeing up to work on this project again soon. I can take a closer look then.

But certainly, on first glance, the changes in PR #80 do appear to resolve this issue.