Thomvis / BrightFutures

Write great asynchronous code in Swift using futures and promises
MIT License
1.9k stars 184 forks source link

callbackExecutionQueue results in EXC_BAD_ACCESS #18

Closed Bouke closed 9 years ago

Bouke commented 9 years ago

I'm trying to perform a background operation using BrightFutures. The result (MKPolyline) should be displayed in a MKMapView; thus the callback should be performed on the main thread. However the callbackExecutionQueue results in an extra dispatch queue on top of the main queue. This results in EXC_BAD_ACCESS when the MKPolyline is added to the MapView. When the highlighted line is removed, everything works as expected.

Simplified usage:

        future(context: Queue.global) { () -> Result<MKPolyline> in
            return Result(polyline)
        }
            .onSuccess(context: Queue.main) { polyline in
                self.mapView.addOverlay(polyline)
            }
Thomvis commented 9 years ago

The block on the callbackExectutionQueue is executed synchronously, so the main thread should still be 'yours'. Can you check if in the onSuccess block NSThread.isMainThread() returns true (it should)? This use case should be covered by the following test: https://github.com/Thomvis/BrightFutures/blob/master/BrightFuturesTests/BrightFuturesTests.swift#L213.

Bouke commented 9 years ago

Ok I hope the comparison of two stack traces helps in this regard. The right one is the default and results in a EXC_BAD_ACCESS, while the left one is my modified version. The default is performed on the main thread, but on the queue named queue serial, while the modified is also performed on the main thread but using the queue named com.apple.main-thread (serial).

callback queue-2

Thomvis commented 9 years ago

Thanks for the additional info. My first idea was that it was an issue with MapKit not supporting non-main thread interaction, but the callback does seem to be on the main thread so that cannot be it?

The self.callbackExecutionQueue.sync is needed to ensure that the callbacks of a single future are executed in serial.

Could you try to experiment with your modified version and see if you can make it crash again. E.g. by wrapping the MapKit instructions in a dispatch_async(dispatch_get_main_thread() or dispatch_sync(custom_serial_queue)` inside the callback. If it is possible, it would be even better if you would have the time to create a failing example project or failing test that I could run myself. Thanks again!

Bouke commented 9 years ago

The main thread is already a serial thread, so maybe an alternate branch could be introduced? That would solve the issue with the main thread I guess.

The main dispatch queue is a globally available serial queue that executes tasks on the application’s main thread.

Thomvis commented 9 years ago

All callbacks should execute on the same serial queue, so that would mean that all callbacks should be executed on the main queue and that is not desired.

I don't think we've yet fully uncovered what causes the crash. You found a solution, but I'm hoping there's an alternate solution that doesn't break current behavior of serial callback execution.

Bouke commented 9 years ago

Some experimentation yields the following results:

Still; as the main queue is a serial thread, it already works FIFO. So why create another serial queue for main thread operations?

Thomvis commented 9 years ago

Thanks again! Did you try those variations with your modified version or the original one? Could you try the first variation with the original BrightFutures? I'd expect that one to succeed, but I am not sure why that is better than without any additional wrapping.

Still; as the main queue is a serial thread, it already works FIFO. So why create another serial queue for main thread operations?

For example:

future {
    return expensive_calculation;
}.onSuccess(context: Queue.main) {
    // callback A: this should happen on the main thread,
}.onSuccess(context: Queue.global) {
    // callback B: this one shouldn't block the main thread
}

In this example, callback A and B should be executed serially, but B should not block the main thread. I believe that the usage of an additional serial queue on top of the callback context is the only way to achieve this, with queues. Or maybe I should try rewriting it with a semaphore or lock.

Thomvis commented 9 years ago

I've added a test for the serial execution of callbacks: https://github.com/Thomvis/BrightFutures/commit/ee68cdc6021c26fd1d7d86f402e88d8055290a27. This test will probably fail with your modifications. The next step is to write a modification (probably using semaphores/locks) that still passes the test but doesn't crash for you. I'll try that tomorrow.

Thomvis commented 9 years ago

A tangential question: is the bad access in your code or inside MapKit? The latter could indicate a threading issue, the former could indicate a different problem. Have you tried running it with zombies enabled, to see what exactly disappeared? Is it self, self.mapView, polyline or something else?

siskaj commented 9 years ago

Hello Thom,

I experience the same problem. I setup a simple example so that you can see the problem. Here is the link:

https://github.com/siskaj/BrightFuturesTest1.git

Thomvis commented 9 years ago

Hi siskaj,

Thank you so much for the information :+1:. I think I now know what is causing the issue. Somewhere deep in MapKit (+[VKDispatch defaultDispatch] to be exact) there is the assumption that MapKit code is executed on the main queue, so not only on the main thread but also on the queue. You can easily reproduce this bug by adding an overlay to a map from the main thread on a private serial queue, e.g.:

var queue = dispatch_queue_create("test", nil)
dispatch_sync(queue) {
    self.mapView.addOverlay(pl!)
}

Even if BrightFutures is not involved at all.

This seems to indicate that we cannot assume that Apple frameworks don't care about the current queue as long as it is on the main thread. It might be a bug (I'll see if I can file a radar), but we can probably write a workaround. I'll see if I can come up with one.

Thanks again, @Bouke too, for bringing this to my attention.

Thomvis commented 9 years ago

I believe the issue has been resolved on master. Could you let me know if it works for you?

siskaj commented 9 years ago

Hi Thomvis

I have tried it and it seems to work fine. Thank you very much for your quick response.