C++: eliminate FSTDispatchQueue and replace it with AsyncQueue

[var-const]

• replace all references to FSTDispatchQueue and FSTTimerID with their C++ equivalents;
• remove FSTDispatchQueue class and related tests;
• in method argument names, replace workerDispatchQueue with just workerQueue, more similar to other platforms;
• move Executor and derived classes out of internal namespace.

[var-const]

This is a large PR, but most of it is deletions/textual substitutions.

[var-const]

_testUserQueue wasn't actually used.

[var-const]

I tried but didn't succeed making this a readonly property.

[var-const]

Using unnamed namespace instead (and const variables are implicitly static anyway).

[var-const]

This should be a tiny improvement, previously the log showed "10.000000 seconds".

[var-const]

Cancel is idempotent.

[rsgowman]

Do you need both weakSelf and self?

[var-const]

The original code uses self implicitly after checking weakSelf, I wanted to minimize the change.

[wilhuff]

The original code only uses self via __typeof__ (which is a nonstandard extension meaning something similar to decltype), which has a compile-time effect but doesn't result in a run-time capture.

Putting self in the explicit capture here retains the strong reference and defeats the purpose of passing this as a weak reference.

Objective-C does not support type inference so __typeof__(self) is the simplest way to say something like "same type just strong". Objective-C++ has auto so you could do this instead:

__strong auto strongSelf = weakSelf;

This is documented as working:
https://stackoverflow.com/questions/44690475/what-are-the-rules-for-deduced-type-by-auto-keyword-when-applied-in-objective

[rsgowman]

This is fine, but my initial reaction was to think there was a bug. You're assigning a 'seconds' value to a variable of type 'millis'. It seems like you'd be off by 1000x. In reality, AsyncQueue::Milliseconds is actually a chrono::duration, so this all works fine.

Not relevant to this PR, but what are your thoughts on changing AsyncQueue::Milliseconds to AQ::Duration? That would change this line to c AQ::Duration timeout = chr::seconds(10); which reads nicer to me. It would also change AQ::EnqueueAfterDelay(Milliseconds delay, ...) to AQ::EnqueueAfterDelay(Duration delay, ...)

[var-const]

Discussed offline, to briefly recap: my concern is that this would make a constructor like AQ::Duration foo{100} non-obvious. <chrono> is very reasonable with its conversions: conversions from larger to smaller units are implicit, but the reverse requires chrono::duration_cast. It's a compromise, but I'm inclined to leave as is.

[rsgowman]

Consider moving these 'using' decl's into the anon namespace, 2 lines down. (http://go/totw/119#scope-of-the-alias)

[var-const]

Done.

[zxu123]

LGTM, only straightforward replacement and should be safe to go.

[var-const]

@wilhuff I have one question on this PR. I made FIRFirestore to be the owner of the worker queue. I suspect FSTFirestoreClient would be a more natural owner; the main reason I've done it that way is to make implementing - (FSTDispatchQueue *)queueForFirestore: in FSTIntegrationTestCase easier. Alternatively, I could add just a getter returning the queue to FIRFirestore, which would delegate to another new getter on FSTFirestoreClient. What do you think?

[wilhuff]

@wilhuff I have one question on this PR. I made FIRFirestore to be the owner of the worker queue. I suspect FSTFirestoreClient would be a more natural owner; the main reason I've done it that way is to make implementing - (FSTDispatchQueue *)queueForFirestore: in FSTIntegrationTestCase easier. Alternatively, I could add just a getter returning the queue to FIRFirestore, which would delegate to another new getter on FSTFirestoreClient. What do you think?

At a high level we should be moving toward FIRFirestore and other FIR-components becoming very thin shells since we'll need to maintain a parallel implementation for the public C++ API.

In terms of the specifics here, it seems reasonable to put the worker queue in its natural home and then add whatever plumbing is required to make the tests work.

It's possible to put such accessors in a Testing or internal category so that they're not a part of the public API. For example, here's an extra test-only accessor on the sync engine:

https://github.com/firebase/firebase-ios-sdk/blob/master/Firestore/Example/Tests/Core/FSTSyncEngine%2BTesting.h

[wilhuff]

The original code only uses self via __typeof__ (which is a nonstandard extension meaning something similar to decltype), which has a compile-time effect but doesn't result in a run-time capture.

Putting self in the explicit capture here retains the strong reference and defeats the purpose of passing this as a weak reference.

Objective-C does not support type inference so __typeof__(self) is the simplest way to say something like "same type just strong". Objective-C++ has auto so you could do this instead:

__strong auto strongSelf = weakSelf;

This is documented as working:
https://stackoverflow.com/questions/44690475/what-are-the-rules-for-deduced-type-by-auto-keyword-when-applied-in-objective

[wilhuff]

This lambda escapes the current stack frame so I'm wary of a default capture like this. I don't feel strongly though and likely non-harmful (default capture by reference would definitely not be OK).

I realize you've probably considered this, but is it worth adopting a convention of spelling out the capture when the lambda escapes?

[googlebot]

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

[googlebot]

CLAs look good, thanks!

[var-const]

At a high level we should be moving toward FIRFirestore and other FIR-components becoming very thin shells since we'll need to maintain a parallel implementation for the public C++ API.

In terms of the specifics here, it seems reasonable to put the worker queue in its natural home and then add whatever plumbing is required to make the tests work.

It's possible to put such accessors in a Testing or internal category so that they're not a part of the public API. For example, here's an extra test-only accessor on the sync engine:

https://github.com/firebase/firebase-ios-sdk/blob/master/Firestore/Example/Tests/Core/FSTSyncEngine%2BTesting.h

(sorry, tried rebasing, which improved history but messed up the comments)

There's a problem with making FSTFirestoreClient own the queue. First, the client's shutdown (omitting unimportant stuff):

- (void)shutdownWithCompletion:(nullable FSTVoidErrorBlock)completion {
  _workerQueue->Enqueue([self, completion] {
    [self.remoteStore shutdown]; // Calls Datastore::Shutdown
  });
}

Once the delayed operation completes, the last reference to FSTFirestoreClient dies, which leads to the destruction of the client and consequently of the worker queue. The problem is that it's possible that in-between when shutdownWithCompletion enqueues the delayed operation, and when the delayed operation is executed by the worker queue, Datastore happens to take another gRPC completion off the queue and enqueue its Complete method. libdispatch will then attempt to run that after the queue is destroyed, because it was enqueued later than shutdown. The cancellation mechanism in ExecutorLibdispatch only deals with delayed operations, not operations scheduled for "immediate" execution.

This didn't affect the previous version of the code, where FIRFirestore owned the queue, because the tearDown method in FSTIntegrationTestCase waits for shutdown to complete (including async operations) before releasing its pointer to FIRFirestore. I'm not sure if user code is required/expected to do the same thing.

The easiest workaround is:

- (void)shutdownWithCompletion:(nullable FSTVoidErrorBlock)completion {
  _workerQueue->Enqueue([self, completion] {
    [self.remoteStore shutdown];
    _workerQueue->EnqueueRelaxed([self] {}); // Hack
  });
}

Needless to say, it's pretty hacky. I'll give it some more thought tomorrow; what do you think?

[var-const]

The original code only uses self via __typeof__ (which is a nonstandard extension meaning something similar to decltype), which has a compile-time effect but doesn't result in a run-time capture.

Putting self in the explicit capture here retains the strong reference and defeats the purpose of passing this as a weak reference.

Objective-C does not support type inference so __typeof__(self) is the simplest way to say something like "same type just strong". Objective-C++ has auto so you could do this instead:

__strong auto strongSelf = weakSelf;

This is documented as working:
https://stackoverflow.com/questions/44690475/what-are-the-rules-for-deduced-type-by-auto-keyword-when-applied-in-objective

Thanks for spotting the self problem (looking at weakSelf and strongSelf, I mistakenly treated self as if it were this).

When calling _workerQueue->Enqueue, is another weakSelf necessary?

[wilhuff]

Treating self and this as equivalent isn't really problematic--they're essentially functionally equivalent.

The difficulty in translating Objective-C to C++ is that when it comes to any Objective-C reference (self or otherwise), any assignment to a local or parameter could extend the lifetime of the object. (However, in the interest of not giving the wrong impression the self passed implicitly to a method on invocation is unsafe_unretained: Objective-C assumes the caller of a method on an object retains the reference).

One weakSelf is enough. There's nothing magical about which weak reference we're using: all the strong and weak references to a given object share the same control block, no matter how they're created.

[var-const]

@wilhuff Hmm, I don't think this works as intended -- because initialized is copied into the lambda, it seems like line 143 will always take the "true" branch. Looking through history, it appears that in a previous iteration, this was a __block variable captured in a block. I don't know the rules for blocks, but unless they somehow extend the lifetime of locals, perhaps it suffered from the same problem.

[wilhuff]

The credential change listener is lambda that's called multiple times. When the lambda is first created initialized is copied into its closure and on the first call the value of initialized is flipped to true. The closure is preserved from invocation to invocation which makes this work.

The prior version which used a __block used the same principle though it works differently. When you decorate a variable with __block it's allocated directly in the closure and then the local acts like a non-const reference. The net effect is the same though, the initial value of initialized only affects the first invocation of the block.

If this weren't working as intended (i.e. that assigning initialized = true never had any effect) we'd attempt to set_value on the userPromise more than once which would result in a std::future_error which we're not seeing.

[var-const]

Right, it's the same function that is invoked each time. Sorry, false alarm.

[var-const]

This lambda escapes the current stack frame so I'm wary of a default capture like this. I don't feel strongly though and likely non-harmful (default capture by reference would definitely not be OK).

I realize you've probably considered this, but is it worth adopting a convention of spelling out the capture when the lambda escapes?

I think it's reasonable to mandate explicit capture list in this case (except in tests). Updated throughout.

[wilhuff]

The easiest workaround is:

- (void)shutdownWithCompletion:(nullable FSTVoidErrorBlock)completion {
  _workerQueue->Enqueue([self, completion] {
    [self.remoteStore shutdown];
    _workerQueue->EnqueueRelaxed([self] {}); // Hack
  });
}

Needless to say, it's pretty hacky. I'll give it some more thought tomorrow; what do you think?

The traditional way to deal with the problem in Objective-C and libdispatch is to capture the thing you're calling back into with a weak reference. That way if the object you're calling back into is destroyed while you're waiting your turn on the queue you can detect it and no harm done.

The alternative arrangement, used when the deferred work should keep the target object alive, is to capture a strong reference.

The scenario you're describing here seems essentially the same and it seems as if this could be straightforwardly be solved by making the GrpcCompletion keep a std::weak_ptr to the async queue. Unfortunately this change is anything but straightforward since the async queue currently is referred to by a non-owning raw pointer which cannot convey changes in lifetime. It would be invasive to do this.

Meanwhile, the approach you've given above still seems like it would work for gRPC but it only works because our implementation finds and destroys all pending operations before returning. It wouldn't work in the arbitrary callback case like where Auth is calling us back.

Meanwhile @rsgowman was complaining about something similar on Android. Is it worth investigating adding general support to the async queue to handle this cleanly?

[var-const]

Ok, so the easiest way to make FSTFirestoreClient own the queue and avoid lifetime issues is to remove the bit in FIRFirestore shutdown where it lets go of its reference to client early. This fixes integration tests.

Two approaches I can think of in general would be:

• having a method in AsyncQueue that, after it is invoked, will make the queue stop accepting new operations;
• making the executor cancel all operations, not just delayed operations, in its destructor.

I'd prefer not to do either as part of this PR. The latter approach seems reasonable to me, but implementing it in ExecutorLibdispatch is not straightforward (ExecutorStd, on the other hand, doesn't suffer from this problem at all).

[var-const]

Right, it's the same function that is invoked each time. Sorry, false alarm.

[wilhuff]

LGTM

I'm OK with deferring major rework here. We'll likely need to revisit whatever we do here once we try to support app deletion so conserving effort in the short term is the right call.