Calls snapshotter (Obj-C) completion block on dealloc if snapshot hasn't finished.

[julianrex]

The completion block passed to -[MGLMapSnapshotter startWithQueue:completionHandler:] should be called regardless of whether the snapshot has finished or not.

This change adds that (along with some tests), calling the completion block with an NSError if the snapshot process has not completed. (Alternatively, an exception could be raised and was considered.)

This should hopefully make cases like #12336 clearer as to what is happening.

[ChrisLoer]

Overall this looks like a great improvement! Couple questions:

• Instead of calling the variables "queue"/"originQueue", shouldn't they both be called something like "responseQueue"? It's certain to be the queue we're sending a response to, but it may not be the origin.
• Shouldn't cancel() also clear the completion/queue values? The completion block (which may capture important resources) was previously owned by _snapshotCallback so it should probably still have the same lifetime. Ideally maybe there'd be a way to tie them together? Are there any other areas we need to audit now that the lifetime of the completion handler is not limited to arguments/block-capture?
• Does it matter that we hold onto a reference to the queue indefinitely long after the callback is finished? I think the answer is probably no, but I don't know all the ways dispatch queues can be created and what their lifecycle is supposed to look like.

[julianrex]

Instead of calling the variables "queue"/"originQueue", shouldn't they both be called something like "responseQueue"? It's certain to be the queue we're sending a response to, but it may not be the origin.

Good suggestion - I'll make that change.

Shouldn't cancel() also clear the completion/queue values?

Yep good catch, and we should call the completion with a "cancelled" error too.

The completion block (which may capture important resources) was previously owned by _snapshotCallback so it should probably still have the same lifetime. Ideally maybe there'd be a way to tie them together? Are there any other areas we need to audit now that the lifetime of the completion handler is not limited to arguments/block-capture?

Yes, this probably needs a deeper look. But I think if we need to make changes there it could be part of a second PR.

Does it matter that we hold onto a reference to the queue indefinitely long after the callback is finished? I think the answer is probably no, but I don't know all the ways dispatch queues can be created and what their lifecycle is supposed to look like.

I think we're good to release the reference to the queue in dealloc.

[ChrisLoer]

Next tricky question: how should we deal with backwards compatibility here? I think executing the callback when the request is cancelled makes sense as a general design, but it's not the current interface, and current users will probably be passing in completion blocks that don't expect to have to handle an error when they call cancel().

[julianrex]

The alternative would be to pass nil for both the snapshot result and the error, but I prefer being explicit here. I think this is only solved with documentation... and a change log entry 😉

[ChrisLoer]

OK, I'll defer to someone on the iOS team on the backwards-compatibility call. I think in core we're maybe a little more shy about breaking changes since there are so many downstream consumers it's hard to get all the deprecation/communication decisions right.

[ChrisLoer]

Specifically, the requirement here is that you can't start the snapshotter from a thread that doesn't have its own RunLoop, right? Thinking back to #12336 (comment), is there a way we can support starting from an arbitrary dispatch queue by allowing the user to provide an existing RunLoop. What I'm thinking of is how some of the complexity for the OP was due to calling the code from an async dispatch block on the main queue -- when really the only reason it had to run on the main queue was to get mbgl::Scheduler::GetCurrent(), right?

[julianrex]

When I looked back at this code it was definitely confusing as to what was going on, which is why I updated the "assert" comment above.

is there a way we can support starting from an arbitrary dispatch queue by allowing the user to provide an existing RunLoop

This is getting out of my knowledge area 😄 but I agree that providing such a mechanism would be handy, as currently we can only ever call start.. from the main queue. What's the process of creating a RunLoop for a dispatch queue?

[ChrisLoer]

🤔 OK I recognize this is a lot of scope expansion, but I think it might be worth taking this on while we're motivated by the original bug report and we've got our heads wrapped around what we expect from these tests.

Thinking about it a little more, I don't think we really want to create a RunLoop on the dispatch queue (although you could). The current RunLoop is just what we use to get a Scheduler to pass to the MapSnapshotter::Callback. It needs someone to process the callback completion message -- but I don't think there's actually a requirement that the callback be processed on the main/UI thread. Instead of passing *mbgl::Scheduler::GetCurrent(), we could pass mbgl::sharedThreadPool(), so the completion would be handled on one the worker threads we already have set up. This would also remove the necessity of the extra step where we do drawAttributedSnapshotWorker on a separate worker queue, since we'd already be on a worker (albeit the mbgl shared worker pool instead of a GCD worker queue).

The complication here is that we need to make sure we don't modify the MGLMapSnapshotter object (aka strongSelf) from a thread that's different from the one that owns it. That was effectively accomplished before by forcing the callback to run on the main thread. Most of the isolation we need is already accomplished by drawAttributedWorkerSnapshot, we just have to move the capture of things like strongSelf.options.scale up another level.

The tricky part is handling the logic for the destruction of the _snapshotCallback (and allowing the calling thread to create a new one). I think you can do the destruction of _snapshotCallback from the background, but do it before you call the completion handler, and before you set strongSelf.loading = false. You could set loading to be atomic, too. That way, you could be guaranteed that whoever the calling thread was, it wouldn't try to create a new callback until the old one was torn down. Also we want to make sure we're done with the tear down before we queue the completion callback, because the completion callback is likely to run on the thread that owns the MGLMapSnapshotter, and whoever receives the completion is likely to assume they can immediately queue another request.

[fabian-guerra]

While I think providing more context about why an action was cancelled or deleted is a good idea. I agree with @ChrisLoer that this may break current code because we have not documented that cancelling or deleting the snapshot will get completionHandler called when executing the following method:
- (void)startWithCompletionHandler:(MGLMapSnapshotCompletionHandler)completionHandler;

This change may be a candidate for version 5 —as all breaking changes goes into major releases—.

Have you considered adding a status property? Similarly to what NSOperation does in this state example. With this change we may not have to wait until v5, and we still provide context about what happened.

[fabian-guerra]

Shouldn't this test for _loading instead?

[julianrex]

It probably should check for _completion - but it's very 50/50, since these things are currently tied together. I suspect that since I added the completion property we might be able to remove loading (and make the completion block nonnull).

[fabian-guerra]

Prefer the one line if format. To avoid future confusions. if (!strongSelf.completion) return;

[ChrisLoer]

FWIW, mbgl-core prefers the verbose guard style:

if (condition) {
   return;
}

I think the reasoning is basically to standardize control flow syntax as much as possible so that people skimming code don't miss a hidden statement tacked onto the end of a line. Also it's handy to be able to quickly put a breakpoint that hits on the return condition.

But I don't have strong evangelical feelings here and it's reasonable for our Objective-C code to follow its own style.

[julianrex]

I tend to prefer multi-line for debugging purposes. Slip of the finger, not adding the braces.

[fabian-guerra]

We have been following the one line syntax or using brackets for one line ifs. While this seem minor it can hide potential bugs in the future. I think either

if ( condition ) {
     return;
}

or
if ( condition ) returns;
Achieves the goal.

[julianrex]

While I think providing more context about why an action was cancelled or deleted is a good idea. I agree with @ChrisLoer that this may break current code because we have not documented that cancelling or deleting the snapshot will get completionHandler called [...]

This is less about providing context and more about ensuring that the completion block gets called in both cases; the fact it currently doesn't I would consider a bug.

Although it's not documented that cancel (or dealloc) would call the completion block, I believe a developer would assume that the block would always be called (by convention), since -startWithQueue:completionHandler: states

@param completionHandler The block to handle the result in.

If we decide that this fix should go in a v5.0.0, then we should clearly document the cases where the completion block isn't called.

Have you considered adding a status property?

A KVO-able status property could an interesting addition and a stepping stone to v5.0.0. @fabian-guerra do you know if KVO works from a dealloc method?

[fabian-guerra]

If we decide that this fix should go in a v5.0.0, then we should clearly document the cases where the completion block isn't called.

I agree with that. It may be more useful for the time being to clarify the behavior for when the block is called. And leave this PR for a major semver.

A KVO-able status property could an interesting addition and a stepping stone to v5.0.0. @fabian-guerra do you know if KVO works from a dealloc method?

I'm not sure calling KVO from a dealloc makes sense. I had the idea that the goal for this PR was provide execution context, that's why I was asking if a status property approach would be something worth to take a look. And if it does then ready->running->finished and cancelled are state options but 'dealloc'.

[julianrex]

I had the idea that the goal for this PR was provide execution context...

No, it's really to solve a hang and potential crashes. Any additional context is icing on the cake.

[julianrex]

I removed the "cancelled" NSError, and replaced with a (currently private) cancelled property. I also removed the loading property as the completion block plays this role.

I consider this PR to be a bugfix - the completion block should be called in all situations to ensure that we don't end up with retain cycles (and to ensure that client code is called). As such I hope that we can find a way for this to make it into the next minor release.

We could change this so that the completion block is NOT called when cancel is called - and leave it up to the developer to manually call the block (they'd need to do some block management of their own). We could document this as a known issue, but...

The situation is worse when the snapshotter is being deallocated as there is no callback (other than the completion block) for the user to hook into.