Notify when db read transaction is advaced.

[nicola-cab]

What, How & Why?

Implement a mechanism for notifying SDKs when db read transaction version is bumped.
Fixes: #5246

☑️ ToDos

• 📝 Changelog update
• 🚦 Tests (or not relevant)
• C-API, if public C++ API changed.

[nicola-cab]

I am not sure about naming, but this should do, in order to notify SDKs when DB read transaction is advanced. I still need to add tests around this. Furthermore, I have just infer what I think it is needed inside core to implement this feature.

[nirinchev]

Is the intention for this to be C API-only feature? Most SDKs still use the C++ API, so it would be beneficial to have it exposed in C++ as well.

[nicola-cab]

Yep, I am trying to understand where the other callbacks are added following what we do in the coordinator...

[nicola-cab]

Yep, I am trying to understand where the other callbacks are added following what we do in the coordinator...

OK, it seems to me that the BindingContext is an object which implementation we defer, in case of the CAPI we have a wrapper for it, but it seems to me that other C++ consumers are probably implementing this in the SDK layer that is consuming object store.

The context factory is set via:
void SyncUser::set_binding_context_factory(SyncUserContextFactory factory)
Is this the case @tgoyne?

If this is the case, we can either import the Context implementation inside core (if it does not contain any platform specific code) or maybe define a new virtual function for registering a callback (although this won't solve the problem, since the logic has been put inside did_chage(), but this can be changed).

[nicola-cab]

@tgoyne can you please advise on how cocoa implements ContextBinders? Thanks ..

[tgoyne]

BindingContext is there specifically to expose hooks to the SDKs and inherently can't be implemented in core (we used to call the SDKs "bindings" and the type was never renamed). https://github.com/realm/realm-swift/blob/master/Realm/RLMRealmUtil.mm#L98 is Cocoa's implementation (which isn't terribly interesting since just calls the actual implementations of things).

SyncUser::set_binding_context_factory() is unrelated. It is a similar concept for a different place where the SDK needs to implement some of the functionality.

[nicola-cab]

BindingContext is there specifically to expose hooks to the SDKs and inherently can't be implemented in core (we used to call the SDKs "bindings" and the type was never renamed). https://github.com/realm/realm-swift/blob/master/Realm/RLMRealmUtil.mm#L98 is Cocoa's implementation (which isn't terribly interesting since just calls the actual implementations of things).

SyncUser::set_binding_context_factory() is unrelated. It is a similar concept for a different place where the SDK needs to implement some of the functionality.

Ok, but this means that we can't have this logic for notifying the SDKs that are consuming core via C++ built inside did_change(), since the implementation might be different between SDKs.

Unless you want to code this inside Cocoa and JS will have to do the same,
Probably we should/could add a new "hook" method that is invoked if there's going to be a pending refresh and expose a static ObjectStore API in order to set the "expected" version.

[cmelchior]

There is already realm_add_realm_changed_callback which Kotlin uses. Is that not sufficient?

[nicola-cab]

There is already realm_add_realm_changed_callback which Kotlin uses. Is that not sufficient?

I believe we are trying to solve a slightly different problem.
#5246 (comment)

[nirinchev]

For context, the problem with realm_add_realm_changed_callback is that it will fire the next time the Realm is changed - instead, we want a callback that fires when the Realm is advanced to the latest version - that is, either immediately if there are no new versions on other threads, or whenever it advances, in which case, it will be like a one-off notification callback.

[nicola-cab]

I think we need to agree on when to put this changes, if we do it inside did_change the SDKs that consume core will have to implement in their own code base, otherwise we could add a new hook in the BindingContext realm_advanced() and add a new object store activation API ObjectStore:notify_when_realm_advanced() @tgoyne

[tgoyne]

This doesn't do the requested thing. invoke_if is comparing a version number to the identifier token for the callback, which aren't sensible things to compare.

SDKs which are using BindingContext directly rather than the C API will indeed have to implement whatever thing it is they want to do. The BindingContext API leaves it up to the SDK to manage callbacks, and changing that should be part of a larger project to redesign/remove BindingContext entirely.

[nicola-cab]

This doesn't do the requested thing. invoke_if is comparing a version number to the identifier token for the callback, which aren't sensible things to compare.

SDKs which are using BindingContext directly rather than the C API will indeed have to implement whatever thing it is they want to do. The BindingContext API leaves it up to the SDK to manage callbacks, and changing that should be part of a larger project to redesign/remove BindingContext entirely.

@tgoyne fixed the erroneous comparison between apples and pears (let me know what you think). Please, a look again. The only thing I am not 100% sure is that the C API consumers will have to register this listener while within a write transaction, which seems a bit odd. Otherwise, they won't be able to get the last valid snapshot (since no transaction would be in progress).
However, maybe there is another good place where to put the activation code to fill up the list of callbacks.

[tgoyne]

We can't use .value() because it requires too high of a deployment target, and it's not needed because the optional not having a value is checked above.

[nicola-cab]

would * operator work here?

[nicola-cab]

@tgoyne can we get at the bottom of this review. I think this is ready to be merged.

[ironage]

I think this might be a good place to check if (realm->is_frozen()) and return an error or nullptr.

[ironage]

LGTM