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

Add write transactions which do not produce local notifications #3665

Closed
jpsim opened this issue May 28, 2016 · 16 comments

Comments

Projects
None yet
5 participants
@jpsim
Copy link
Contributor

commented May 28, 2016

The current fine-grained notifications approach makes it challenging to support scenarios where the UI is updated before the data model, rather than the reactive approach of the model being the canonical source of truth that the UI merely displays. The simplest motivating use-case is UITableView reordering; when using the built-in functionality for dragging rows around the delegate is merely notified of changes already displayed in the UI, and re-applying those changes in the collection notification block after the Realm is committed will corrupt the order to the UITV. Because notifications are coalesced, users can't reasonably filter out these redundant updates themselves. and even if they could it would be very easy to get wrong.

We should expose a variant of Realm.write(_:) that would specify that the transaction should not produce notifications on the current thread, as the things they want to update with the notifications are already up-to-date. In addition, this variant of write transactions would block until all async notifications are ready and delivers them, since otherwise the UI could easily get "out of sync".

An alternative approach would be to instead let the user pass in the tokens for notification blocks which they want to suppress. This is more flexible and makes it work with commits on a different thread from the notified thread, but maybe would make the API even more complex.

@tgoyne has implemented a proof of concept of this in #3439, and there's more discussion in that thread, but that work is currently on hold.

@tgoyne

This comment has been minimized.

Copy link
Member

commented May 31, 2016

My current thoughts on this:

Beginning a write transaction should always wait for notifications to be ready and send them if there are any for the current thread. Only doing this sometimes is weird, and in the scenarios where you actually need to be doing the write transaction on the thread with notifications registered, you probably also need your local view of the data to be up-to-date. Async writes should address the case that this would hurt where write transactions are done on the main thread out of laziness.

Skipping a notification should be done on a per-token basis from within the write transaction; something like [token skipNotificationForCurrentTransaction] or [realm skipNotificationForToken:token].

@jpsim

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2016

100% agreed with your first paragraph.

Putting the transaction skip action on the token is an interesting idea that I hadn't considered, but it feels like it makes our APIs more stateful than they are now, especially since it reduces the locality of state changes. That's why I'm still more in favor of func beginWrite(ignoringNotificationTokens: [NotificationToken] = []) and func write(ignoringNotificationTokens: [NotificationToken] = [], block: () -> ()), even though that also has compromises.

@JadenGeller

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2016

Do we only imagine this API applying to RLMCancelationTokens or also RLMRealmNotificationTokens? It doesn't seem to be very useful for the latter and the implementation of each seems pretty different.

If it should only apply to RLMCancelationTokens, we'll need to publicly expose their type (as right now we only expose the superclass RLMNotificationToken), and it might be a good idea to rename it to something collection-related while we're at it.

If it should apply to both, we need to specifically outline what we expect ignoringRLMRealmNotificationToken to do?

//cc @tgoyne @jpsim

@tgoyne

This comment has been minimized.

Copy link
Member

commented Jul 13, 2016

I don't see how there's any ambiguity over what suppressing a realm-level notification would do, and while it doesn't seem useful at all, the implementation would be completely trivial so there's no reason not to support it for consistency.

@JadenGeller

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2016

Would both DidChange and RefreshRequired be ignored, or just DidChange?

@tgoyne

This comment has been minimized.

Copy link
Member

commented Jul 13, 2016

RefreshRequired isn't sent for writes on the current thread, as the Realm is inherently already at the latest version when it's committing a write transaction.

@emuye

This comment has been minimized.

Copy link

commented Jul 20, 2016

Is there any info about when this will be available?

@jpsim

This comment has been minimized.

Copy link
Contributor Author

commented Jul 20, 2016

Is there any info about when this will be available?

It's at least a few weeks away, maybe longer. I do think that @JadenGeller wants to start working on this soon.

@JadenGeller

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2016

Correct. I've started looking into how it might be done, but I haven't begun implementation.

@JadenGeller

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2016

The implementation I have in mind has a precondition that the token parameters to write(skippingNotificationFor:) be associated with calling thread. Is this a reasonable limitation?

@jpsim

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2016

I think so. That's sufficient for the principal case driving this feature where you want to ignore notifications on the main thread for writes that have already been reflected in the UI.

@JadenGeller

This comment has been minimized.

Copy link
Contributor

commented Jul 28, 2016

This API will be tricky to name. A method named [RLMRealm beginWriteTransactionSkippingNotificationForTokens:] is confusing in that it will block until notifications can be flushed for the specified tokens despite the name suggesting that no notifications will be delivered for these tokens.

@tgoyne

This comment has been minimized.

Copy link
Member

commented Jul 28, 2016

Indicating which tokens to skip can be done on commit, which makes the behavior a lot less weird. Doing it on begin was only necessary for the logic of conditionally blocking for previous notifications.

@jpsim

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2016

Unassigned @JadenGeller based on our conversation from earlier.

@timshadel

This comment has been minimized.

Copy link

commented Nov 2, 2016

Can you help me ensure I understand the problem correctly, and the risks with my really naive approach?

I have a basic function that unsubscribes/runs an action/resubscribes to notifications.

func watchEntries(action: (() -> ())? = nil) {
    let newSubscription = (notificationToken == nil)
    notificationToken?.stop()
    action?()
    notificationToken = entries?.addNotificationBlock { changes in
        guard let tableView = self.tableView else { return }

        switch changes {
        case .initial:
            if newSubscription {
                tableView.reloadData()
            }
        case .update(_, let deletions, let insertions, let modifications):
            tableView.beginUpdates()
            tableView.insertRows(at: insertions.map({ IndexPath(row: $0, section: 0) }),
                                 with: .automatic)
            tableView.deleteRows(at: deletions.map({ IndexPath(row: $0, section: 0)}),
                                 with: .automatic)
            tableView.reloadRows(at: modifications.map({ IndexPath(row: $0, section: 0) }),
                                 with: .automatic)
            tableView.endUpdates()
        case .error(let error):
            fatalError("\(error)")
        }
    }
}

Then I stop notifications while I run the delete.

func tableView(_ tableView: UITableView, commit editingStyle: UITableViewCellEditingStyle, forRowAt indexPath: IndexPath) {
    if let entry = entries?[indexPath.row] {
        watchEntries() {
            tableView.beginUpdates()
            tableView.deleteRows(at: [indexPath], with: .automatic)
            try! self.realm?.write {
                self.realm?.delete(entry)
            }
            tableView.endUpdates()
        }
    }
}

This seems to work, but I suspect that because the "pause" on notifications isn't exactly tied to the write transaction itself, I run the risk of missing notifications from other theads that occur in the background. On re-subscribing I get the .initial case but ignore it so that the table animation works. If I don't use any background threads, is this a reasonably accurate approach until the official feature is done?

@tgoyne

This comment has been minimized.

Copy link
Member

commented Nov 2, 2016

Yes, that works fine as long as you can guarantee that no write transactions which make changes relevant to your query will occur anywhere in your app between your write and when you get the .initial result, including further writes on the main thread. This is hard to guarantee in a complex app (and impossible with RMP, which is the motivating factor we had to dance around in the initial writeup), but if you can guarantee it in your specific use-case then you'll be fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.