Firebase-ios-sdk: Swift `observeSingleEvent` closure issue.

[ryanwilson]

Describe your environment

• Xcode version: N/A
• Firebase SDK version: 4+
• Library version: 4.0.2
• Firebase Product: database

Describe the problem

It's difficult to observe a single event without getting a cached value in Swift. The value is cached, leaving the opportunity to retrieve stale data. This can usually be mitigated by using the removeObserver(withHandle:) call, but that doesn't work well in Swift due to the way closures work.

Steps to reproduce:

Original issue.

1. Set Database's persistence enabled flag to true (Database.database().isPersistenceEnabled = true)
2. Call observeSingleEvent on a reference from Database.
3. Change the value on the server.
4. Call observeSingleEvent again, retrieve old stale value.

Attempted solution.

1. Call observeEvent on a reference, assign the return value to a var.
2. Attempt to call removeObserver(withHandle:) in the closure.
3. Compiler error. _"Variable used within it's own initial value."_

Relevant Code:

let ref = Database.database().reference()
var handle = ref.observe(.value) { (snapshot) in
  ref.removeObserver(withHandle: handle) // Compiler error on 'handle' usage.
}

Workaround

If the ref only has a single observer, ref.removeAllObservers() will work in place of the removeObserve(withHandle:) call. Unfortunately, this doesn't work if multiple observers are added to the ref.

Adding a property declaration should work, but isn't ideal as it pollutes the class or struct being used, leaves some uncertainty whether or not the removeObserver call will actually happen.

// Property declaration
var handle: DatabaseHandle?

func readFromDatabase() {
  let ref = Database.database().reference()
  handle = ref.observe(.value) { [weak self] (snapshot) in
    guard let handle = self?.handle else { return }

    ref.removeObserver(withHandle: handle)
  }
}

Potential Solution(s)

Providing a parameter on the observeSingleEvent method that bypasses the cache and will only fetch from the server is a possibility. Some naming thoughts: bypassCache forceFetch, fromServer.

[morganchen12]

Can you not avoid the compiler error by changing the handle declaration from a let to a var?

let ref = Database.database().reference()
var handle = ref.observe(.value) { (snapshot) in
  ref.removeObserver(withHandle: handle)
}

[ryanwilson]

@morganchen12 nope, same issue. Swift tries to capture the value before it's initialized. I can change the sample code to a var so it's a bit more clear!

[BigSauce]

Thanks for creating this issue, @ryanwilson.

Important Note:**
observeSingleEvent will retrieve cached values only when persistenceEnabled = true.

I would modify the Steps to Reproduce from:

Steps to reproduce:

Original issue.

1. Call observeSingleEvent on a reference from Database.
2. Change the value on the server.
3. Call observeSingleEvent again, retrieve old stale value.

to

1. Set Database's persistence enabled flag to true (Database.database().isPersistenceEnabled = true)
2. Call observeSingleEvent on a reference from Database.
3. Change the value on the server.
4. Call observeSingleEvent again, retrieve old stale value.

In cases where persistence is enabled, Swift consumers of the Firebase SDK have no recourse for retrieving a fresh Firebase node value other than storing a node handle (UInt returned by observeEvent) as a property, which is very impractical. Another workaround is to create a trampoline interface in Objective-C for Swift consumers to use, but this is also impractical.

My suggestion would be to add a Bool parameter to observeSingleEvent (maybe called bypassingCache) that defaults to false. When true, observeSingleEvent would always perform a retrieval of the node's value. After retrieval of the node's value, the method would remove associated observers from the DatabaseReference.

The final method signature might look something like this in Objective-C:

- (void)observeSingleEventOfType:(FIRDataEventType)eventType bypassingCache:(BOOL)bypassCache withBlock:(void (^)(FIRDataSnapshot *snapshot))block;

I might recommend adding this as an overloaded method so it doesn't break existing code. If we want to support Swift default values, then bypassingCache bypassCache: Bool = false would need to be the last parameter in the function, instead of where I listed it in the method declaration.

[BigSauce]

Can you not avoid the compiler error by changing the handle declaration from a let to a var?

@morganchen12 I tried that approach, to no avail. There must be something I'm not understanding about how that type is captured, because the following similar pattern works in NotificationCenter:

var token: NSObjectProtocol?
token = NotificationCenter.default.addObserver(forName: NSNotification.Name(rawValue: kNotificationOnlineStatusUpdated), object: nil, queue: OperationQueue.main) { notification in

     // this works

     NotificationCenter.default.removeObserver(token!)
}

I speculate that it must have something to do with the way NSObjectProtocol works or the way optionals are captured.

[morganchen12]

It's because we return a value type (FIRDatabaseHandle) and not a reference type, so the value is copied when captured in the block rather than referenced like in the NotificationCenter example. Using an ivar/property gets around this because the object's members/the variable captured by reference can be modified and then referenced from inside the closure.

Agreed, this is pretty annoying and not immediately clear.

[quanvo87]

@ryanwilson even in your workaround, the callback will fire twice, first returning stale data, and second returning fresh data. I tried the same thing and this had negative downstream implications in my app. I strongly support a new feature to force observeSingleEvent to get fresh data. As of now, observeSingleEvent and persistence are essentially mutually exclusive features.

[morganchen12]

For those struggling with this issue, the cleanest workaround is probably to use a Box reference type. If you don't want to take on another dependency, you can implement your own box pretty easily.

[BigSauce]

Thanks for the advice, @morganchen12. I'll give that route a try.

@ryanwilson, has this issue gained any traction with the dev team?

[morganchen12]

@BigSauce it's a very straightforward problem with an equally straightforward (and easily implemented) solution. The only thing that's prevented us from making this change is it's a breaking API change requiring a major version bump, as the return type of our function must be changed.

[BigSauce]

Gotcha. So you're saying the best time for this to be implemented by the Firebase iOS SDK team is for a major release as opposed to ASAP (due to breaking changes)? If so, that makes sense.

For now I'll try the boxed type workaround and see if it works on my end. In the mean time, I'll reply on this thread if I run into any issues with that, so perhaps others can benefit from my work.

[quanvo87]

One workaround I considered was:

let ref = Database.database().reference()
var handle = UInt.max
handle = ref.observe(.value) { (snapshot) in
  ref.removeObserver(withHandle: handle)
}

One problem I had was:

• we usually aren't retrieving a value from the root of our database
• we build a path with ref.child("etc")
• if we build a path with no value at the end, the observe callback is never fired
• so how do we know when something doesn't exist at the path?

[morganchen12]

@ryanwilson can you tackle this for our next major release?

[ryanwilson]

I'll do my best to get this in for the next major release. I'll have to check if there's enough time to deprecate and remove the old one by the next major release.

[ghost]

Guess there wasn't enough time... This whole persistenceEnabled thing is really genius, but only being able to use it with the permanent observers makes it really hard to work with...

[ryanwilson]

There's still time to get this in and I'm still giving it a shot. Apologies for the delay in response! Contributions are welcome for the implementation side of this, too 😃

[damirstuhec]

Any updates on this?

[Kimnyhuus]

I just ran into this issue because I'd experienced the exact same problem.
With persistence enabled, we cannot check if a child node exists because it only checks locally.

We're currently building a chat for our app.
Each chat corresponds to something called a "challenge".
So if a user creates a challenge which is send to multiple other users, then a chat is created in that same instance.

Because we're rolling out this chat after the app it self was launched, a lot of old challenges won't have a chat. Therefore we need to check if a chat exists or not. This is where "observeSingleEvent" became an issue.

Because we've enabled persistence. And after doing that, it was suddenly only the creator of the challenge on the exact phone it was created from, who could find the chat. When logging in to the same user on another phone, no chat is found and therefore we don't show a chat button, even though we know the chat exists.

My solution to this is:

var tempHandle: DatabaseHandle?
tempHandle = chatRef.child("foo").child("bar").observe(.value) { [weak self] (snapshot) in
    guard let handle = tempHandle, let `self` = self else { return }

    if snapshot.hasChild("someChatKey") {
        self.showChatButton()
        print("Has chat")
    }
    else {
        print("No chat")
    }

    chatRef.removeObserver(withHandle: handle)
}

After testing this, I can see in the console that says "No chat" first, and then "Has chat".
So it works, and we'll go with this for now. (The "else" gets deleted though. Only necessary for testing.)

[Kimnyhuus]

Any update on this?

After updating the Firebase pods on our project, the workaround described in this thread (and in my own comment above this) doesn't seem to work anymore.

Before, the ".observe" returned first the local data and then the online if there were any changes.
Now it only returns the local just as "observeSingleEvent" did when this issue were created.

To solve this issue I had to add a boolean flag, for get both the local and online data:

var tempHandle: DatabaseHandle?
var onlineReturned = false
tempHandle = chatRef.child("foo").child("bar").observe(.value) { [weak self] (snapshot) in
    guard let handle = tempHandle, let `self` = self else { return }

    if snapshot.hasChild("someChatKey") {
        self.showChatButton()
        print("Has chat")
    }
    else {
        print("No chat")
    }

    if onlineReturned {
        chatRef.removeObserver(withHandle: handle)
    }

    onlineReturned = true
}

This however, creates a minor issue because the observer isn't removed unless I receive data twice (offline & online). I'm getting around this by removing the observer on viewDidDisappear.

These are the versions of Firebase we run:
Using Firebase (5.0.1)
Using FirebaseABTesting (2.0.0)
Using FirebaseAnalytics (5.0.0)
Using FirebaseAuth (5.0.0)
Using FirebaseCore (5.0.1)
Using FirebaseDatabase (5.0.0)
Using FirebaseInstanceID (3.1.1)
Using FirebaseMessaging (3.0.0)
Using FirebaseRemoteConfig (3.0.0)

[kostassite]

@ryanwilson I tried to implement a solution that doesn't need a major release as it doesn't change the observe method signature. I implemented

- (FIRDatabaseQuery *)queryWithBypassCache {
    FQueryParams* params = [self.queryParams enableBypassCache];
    return [[FIRDatabaseQuery alloc] initWithRepo:self.repo
                                             path:self.path
                                           params:params
                                    orderByCalled:self.orderByCalled
                             priorityMethodCalled:self.priorityMethodCalled];;
}

a FIRDatabaseQuery method that sets a property in FQueryParams and then when ask FPersistenceManager for a (FCacheNode *)serverCacheForQuery:(FQuerySpec *)query I return an null node

if (query.params.bypassCache) {
        complete = NO;
        node = [FSnapshotUtilities nodeFrom:nil];
    }

so the observer is not called with the empty node and waiting to get called with the new data from the server. If the user is offline, then the observer is not called at all, so dev needs to do a check for .info/connected or have a timeout.

so the call becomes something like this

ref.child("BlaBla").withBypassCache().observeSingleEvent(of: .value, with: { [weak self] (snapshot) in

Do you see any problem and how that maybe break a different part of the project, because persistence is deeply integrated in it?

the commit can be found here https://github.com/kodika/firebase-ios-sdk/commit/5dc0adbaf5d40fb6cf36da6fe201b3a97f9a913d

[ryanwilson]

Thank you for the attempt @kostassite!

I'm not the expert on Database but @schmidt-sebastian should have more context if this is going to be a problem or not.

[schmidt-sebastian]

I think that we can take a lot from the PR you sent over @kostassite. Let me dwell a bit on the API and find suitable APIs for our other platforms as well. If we can avoid a breaking change, then we will be able to release this much faster.

Note that we just implemented this exact feature in Firestore: https://github.com/firebase/firebase-ios-sdk/blob/master/Firestore/Source/Public/FIRDocumentReference.h#L228. This was also done in a minor release.

[Rahish]

I think that we can take a lot from the PR you sent over @kostassite. Let me dwell a bit on the API and find suitable APIs for our other platforms as well. If we can avoid a breaking change, then we will be able to release this much faster.

Note that we just implemented this exact feature in Firestore: https://github.com/firebase/firebase-ios-sdk/blob/master/Firestore/Source/Public/FIRDocumentReference.h#L228. This was also done in a minor release.

So when we can expect the Right solution for this issue? As its causing issues in the project and this issue is open from long ago

[n3at18]

any update on this?

potential solution could be adding a flag to reference itself like "keepSynced" but without downloading the whole tree - it's not acceptable in many cases.

Like "reference.skipCache" flag which defaults to "false"

It shouldn't affect existing code as far as I can see

[schmidt-sebastian]

We are currently in the process of designing a solution for this. This is in the very early stages, but some evidence can be found here: https://github.com/firebase/firebase-js-sdk/pull/3812