Port FSTAsyncQueryListener to C++ #2620

wilhuff · 2019-03-24T17:36:18Z

There are a few things I'm trying to do with this PR:

Implement in C++
Make ownership clearer and less diffuse, using std::unique_ptr where possible
Make the iOS sources more clearly match Android (in particular Refactor Event Listeners firebase-android-sdk#289)

In particular I wanted EventListener and AsyncEventListener to implement the same interface to make this more like the Android port. In addition to being conceptually simpler this also greatly simplifies the ownership model because previously the AsyncEventListener was retained indirectly through the closure it passed into the QueryListener.

I've added an EventListener<T>::Create that creates an EventListener from a lambda, which should leave many cases just as expressive as before.

Unfortunately, until we can use C++14, there's no generalized capture, so in some cases I've elected to explicitly spell out a class that implements EventListener in order to be able to std::move things into its closure. In another case this allows me to share the contents of the closure with the caller in a way that works functionally like Objective-C __block scoped variables.

This goes the rest of the way to addressing #2564, as far as the cycle in getDocumentsWithSource:completion: is concerned. The test in 786e2f1 shows the leak before and it's gone after.

Note the DIFFBASE on #2619. This change depends upon ListenerRegistration existing as a C++ type, because clang does not allow std::promise of any ARC managed values.

... and use it

These are implicitly deleted because DocumentSet is a member and isn't default constructible.

var-const

until we can use C++14

Any thoughts on when that might happen?

Firestore/core/src/firebase/firestore/core/event_listener.mm

Firestore/core/src/firebase/firestore/core/event_listener.h

var-const · 2019-03-25T19:35:07Z

Firestore/core/src/firebase/firestore/core/event_listener.h

+
+template <typename T>
+void AsyncEventListener<T>::Mute() {
+  muted_.store(true);


Optional: unless it's a stylistic preference, I think you can just use the regular assignment syntax muted_ = true.

I had used load/store initially because I dislike hiding important operations behind operators. However, I can see the appeal. I've changed it because you pointed it out.

I understand the rationale, though in this case an argument can be made that any operation on an atomic variable is atomic, so as long as the reader of the code keeps the type in mind, there should be no ambiguity.

var-const · 2019-03-25T19:45:07Z

Firestore/core/src/firebase/firestore/api/listener_registration.mm

+  if (async_listener) {
+    async_listener->Mute();
+    async_listener_.reset();
+    async_listener.reset();


resetting the local variable seems extraneous?

Reseting this local variable has the effect of making the QueryListener the only thing keeping the AsyncEventListener alive, so that it can be destroyed inline with QueryListener.

However, now that I think about it, there's no great way to guarantee that the QueryListener will be destroyed on our queue because the query_listener local retains it. This means that the various EventListener destructors will run on this thread.

In the end I think that's not as bad a thing as I had thought it would be. The QueryListener and the AsyncEventListener are created on the user's thread so it's fine for them to be destroyed there.

Secondarily, now that you've prompted about it, I think that trying to manage which thread destroys an object by carefully managing the lifetime of shared_ptrs is a losing game. If we really care we need to make this contractual, i.e. that the pointed to object, in its destructor submits the real destructor to an executor. That's beyond the scope of this change though, so I'm not doing anything about it here.

So, I've removed the extra resets.

Firestore/core/src/firebase/firestore/api/listener_registration.mm

var-const · 2019-03-25T19:59:04Z

Firestore/core/src/firebase/firestore/core/query_listener.mm

@@ -65,14 +65,14 @@
      RaiseInitialEvent(snapshot);
    }
  } else if (ShouldRaiseEvent(snapshot)) {
-    listener_(snapshot);
+    listener_->OnEvent(snapshot);


Nit: can this be std::move(snapshot)?

No. The snapshot is retained by the query listener, it is std::moved below. The listener gets a copy.

var-const · 2019-03-25T20:12:46Z

Firestore/core/src/firebase/firestore/api/document_reference.mm

+      }
+    }
+
+    void Release(ListenerRegistration&& registration) {


Optional: maybe something like Unblock or Proceed?

How about Resolve (in the spirit of promises being resolved or rejected)?

Sounds good.

var-const · 2019-03-26T18:22:58Z

Firestore/core/src/firebase/firestore/core/event_listener.h

+
+template <typename T>
+void AsyncEventListener<T>::Mute() {
+  muted_.store(true);


I understand the rationale, though in this case an argument can be made that any operation on an atomic variable is atomic, so as long as the reader of the code keeps the type in mind, there should be no ambiguity.

var-const · 2019-03-26T18:24:00Z

Firestore/core/src/firebase/firestore/api/document_reference.mm

+      }
+    }
+
+    void Release(ListenerRegistration&& registration) {


Sounds good.

* Test that shows a leak. * Add EventListener to match Android. * Use AsyncEventListener in FSTQueryListenerTests * Remove FSTAsyncQueryListener * Use EventListener throughout

wilhuff added 19 commits March 21, 2019 15:09

Replace FSTDocumentSet's guts with immutable::SortedSet.

433fec1

Port FSTDocumentSet to C++ DocumentSet.

83d61c7

Replace implementation of FSTDocumentSet with C++ DocumentSet

5666ce5

Update all usages of FSTDocumentSet

83c0687

Remove FSTDocumentSet

614132f

Merge branch 'master' into wilhuff/document-set

d648e35

Add Objective-C++ guard

f46866c

Use absl::c_equal instead of hand-rolling it.

3f45e44

Split SortedContainer out of SortedMapBase

c4fbcfe

... and use it

Remove = default constructors that aren't true.

114fe05

These are implicitly deleted because DocumentSet is a member and isn't default constructible.

Fix NSNotFound -> npos

03847dc

Fix usage of FSTQuery.limit

9d86b35

Port FSTListenOptions to C++

5cbe0ea

Remove usage of FSTListenOptions

b2116b3

Remove FSTListenOptions

59da887

Add QueryListener

2a6685d

Replace FSTQueryListener with C++ QueryListener

b23a5a2

Implement ListenerRegistration in C++

99aed48

Reimplement FSTListenerRegistration with C++ ListenerRegistration

aec554a

wilhuff added the api: firestore label Mar 24, 2019

wilhuff assigned var-const Mar 24, 2019

wilhuff requested a review from var-const March 24, 2019 17:36

googlebot added the cla: yes label Mar 24, 2019

wilhuff added 5 commits March 24, 2019 10:37

Test that shows a leak.

786e2f1

Add EventListener to match Android.

bf2e76a

Use AsyncEventListener in FSTQueryListenerTests

c7a9a3e

Use EventListener throughout

768d0c5

Remove FSTAsyncQueryListener

d52ef3b

wilhuff force-pushed the wilhuff/async-listener branch from 56bdef0 to d52ef3b Compare March 24, 2019 17:37

Review feedback

8dd6bfc

Merge branch 'master' into wilhuff/listen-options

3c5cd51

var-const approved these changes Mar 25, 2019

View reviewed changes

var-const assigned wilhuff and unassigned var-const Mar 25, 2019

wilhuff added 19 commits March 25, 2019 16:38

Merge branch 'master' into wilhuff/listen-options

1f6c482

Merge branch 'wilhuff/listen-options' into wilhuff/query-listener

ed184d6

Merge branch 'wilhuff/query-listener' into wilhuff/listener-registration

9de0d8f

Merge branch 'wilhuff/listener-registration' into wilhuff/async-listener

6e7064f

Add QueryListener

a39902e

Replace FSTQueryListener with C++ QueryListener

ce97b90

objc::unordered_map

03157ff

Add a lint check for Objective-C banned words

ba38b49

Review feedback

d85bf4e

Merge branch 'master' into wilhuff/query-listener

000bc1a

Merge branch 'wilhuff/query-listener' into wilhuff/listener-registration

2835378

Merge branch 'wilhuff/listener-registration' into wilhuff/async-listener

73cfad7

style.sh generated changes

f186fd1

lint fixes

58b4b97

Fix CMake build

f6a7b65

Merge branch 'wilhuff/query-listener' into wilhuff/listener-registration

043db66

Merge branch 'wilhuff/listener-registration' into wilhuff/async-listener

903fbdf

Merge branch 'master' into wilhuff/async-listener

5a2540d

Review feedback

aaa2abe

wilhuff changed the base branch from wilhuff/listener-registration to master March 26, 2019 16:31

Remove ListenerRegistration default constructor

2682adc

var-const approved these changes Mar 26, 2019

View reviewed changes

wilhuff merged commit ba332be into master Mar 26, 2019

wilhuff deleted the wilhuff/async-listener branch March 26, 2019 18:54

firebase locked and limited conversation to collaborators Oct 19, 2019

Port FSTAsyncQueryListener to C++ #2620

Port FSTAsyncQueryListener to C++ #2620

Uh oh!

Conversation

wilhuff commented Mar 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

var-const left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wilhuff commented Mar 24, 2019 •

edited

Loading