daltoniam / SwiftHTTP

Thin wrapper around NSURLSession in swift. Simplifies HTTP requests.
Apache License 2.0
1.88k stars 313 forks source link

Random crash on observer #148

Closed oziade closed 9 years ago

oziade commented 9 years ago

I'm experimenting EXC_BAD_ACCESS crashes. This crash is not reproducible everytime, but happens sometimes for different users. I have the following crash log, which happens sometimes on some SwiftHTTP calls in background thread :

Thread : Crashed: NSOperationQueue 0x16f64f50 :: NSOperation 0x16ec6f00 (QOS: LEGACY)
0  Foundation                     0x264a88ec __NSOQSchedule + 367
1  ???                            0x34bf608c 
2  Foundation                     0x264083ef +[__NSOperationInternal _observeValueForKeyPath:ofObject:changeKind:oldValue:newValue:indexes:context:] + 1270
3  Foundation                     0x26407e0d NSKeyValueNotifyObserver + 88
4  Foundation                     0x26407b67 NSKeyValueDidChange + 346
5  Foundation                     0x263f45d1 -[NSObject(NSKeyValueObserverNotification) didChangeValueForKey:] + 88
6  SwiftHTTP                      0x00a405c0 _TTSf2n_n_n_n_d_i_n_n___TFFC9SwiftHTTP8HTTPTask6createFS0_FTSS6methodOS_10HTTPMethod10parametersGSQGVSs10DictionarySSPSs9AnyObject___17completionHandlerGSQFCS_12HTTPResponseT___GSqCS_13HTTPOperation_U_FTGSQCSo6NSData_GSQCSo13NSURLResponse_GSQCSo7NSError__T_ + 3480
7  SwiftHTTP                      0x00a3e4f8 _TPA__TTSf2n_n_n_n_d_i_n_n___TFFC9SwiftHTTP8HTTPTask6createFS0_FTSS6methodOS_10HTTPMethod10parametersGSQGVSs10DictionarySSPSs9AnyObject___17completionHandlerGSQFCS_12HTTPResponseT___GSqCS_13HTTPOperation_U_FTGSQCSo6NSData_GSQCSo13NSURLResponse_GSQCSo7NSError__T_ + 112
8  SwiftHTTP                      0x00a3e564 _TPA__TTRXFo_oGSQCSo6NSData_oGSQCSo13NSURLResponse_oGSQCSo7NSError__dT__XFo_iTGSQS__GSQS0__GSQS1____iT__ + 56
9  SwiftHTTP                      0x00a3e5e4 _TPA__TTRXFo_iTGSQCSo6NSData_GSQCSo13NSURLResponse_GSQCSo7NSError___iT__XFo_oGSQS__oGSQS0__oGSQS1___dT__ + 80
10 CFNetwork                      0x251e466f __49-[__NSCFLocalSessionTask _task_onqueue_didFinish]_block_invoke + 254
11 Foundation                     0x264a965d __NSBLOCKOPERATION_IS_CALLING_OUT_TO_A_BLOCK__ + 8
12 Foundation                     0x264144d1 -[NSBlockOperation main] + 148
13 Foundation                     0x26406e87 -[__NSOperationInternal _start:] + 774
14 Foundation                     0x264abfc7 __NSOQSchedule_f + 186
15 libdispatch.dylib              0x34660729 _dispatch_queue_drain + 1468
16 libdispatch.dylib              0x3465aaad _dispatch_queue_invoke + 84
17 libdispatch.dylib              0x34661f9f _dispatch_root_queue_drain + 394
18 libdispatch.dylib              0x346633c3 _dispatch_worker_thread3 + 94
19 libsystem_pthread.dylib        0x347bcdb5 _pthread_wqthread + 668
20 libsystem_pthread.dylib        0x347bcb08 start_wqthread + 8

Did anyone experienced this ?

daltoniam commented 9 years ago

Interesting, I haven't seen that before. You are calling a SwiftHTTP operation from a background thread? SwiftHTTP runs on its own background thread via an NSOperation subclass, so it is a background thread starting an NSOperation?

oziade commented 9 years ago

Sorry, i wanted to point that the error was in a background thread, but the operation is called (or added in NSOperationQueue) from the main thread. Most of the time, i just add the operation in the queue to cancel them if the screen is not displayed anymore (cancelAllOperations in deinit of ViewModel).

daltoniam commented 9 years ago

Ah ok makes sense. I started working on a swift-2 refactor and I noticed that the cancel method might not be properly canceling/notifying as an NSOperation subclass should. I might try changing that method to:

    override public func cancel() {
        task.cancel()
        finish()
    }

and see if it fixes the random crashes. If it does, I will update both the branches to behave properly.

oziade commented 9 years ago

It sounds great. I finally could reproduce quite reliably the crash with the current version (with a completion field, where each character typed cancel all operations of the queue before adding a new operation). Then, I tried with your code above, and i could not reproduce the crash anymore. I did not remove the current call to super (do i have to remove it ??). So here is the code i tried :

    override public func cancel() {
        super.cancel()
        task.cancel()
        finish()
    }

Thanks for the quick answer !

daltoniam commented 9 years ago

Sweet! Glad to hear that fixed, shocked it was wrong for so long (refactors can be good :smile: ). I pushed up a fix for both branches, thanks for the report!