Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Crash under FoundationStream.cleanup() -> () (Starscream.swift) #173

Closed
shawnce opened this issue Mar 1, 2018 · 12 comments
Closed

Crash under FoundationStream.cleanup() -> () (Starscream.swift) #173

shawnce opened this issue Mar 1, 2018 · 12 comments

Comments

@shawnce
Copy link

shawnce commented Mar 1, 2018

What is the issue?

Recently added PusherSwift to our app replacing the older Objective-C library we had. We are seeing a number of users hitting the following crashes. It looks like in general the PusherSwift code doesn't have much in it to deal with concurrency despite having code calling into it from multiple queues / threads. I believe that could be playing into these crashes.

In my use of PusherSwift I am currently ensuring any and all calls I make into it are happening on the main thread given missing concurrency support in the code.

Is it a crash report? Submit stack traces or anything that you think would help

EXC_BREAKPOINT
Crashed: com.apple.main-thread
0  CoreFoundation                 0x1841192c0 CFHash + 360
1  CoreFoundation                 0x184116160 CFBasicHashFindBucket + 164
2  CoreFoundation                 0x1841160a0 CFDictionaryGetValue + 224
3  CoreFoundation                 0x18418f594 _CFStreamUnscheduleFromRunLoop + 184
4  CoreFoundation                 0x18420dcc0 _CFStreamSetDispatchQueue + 168
5  PusherSwift                    0x101a287a8 FoundationStream.cleanup() -> () (Starscream.swift:529)
6  PusherSwift                    0x101a29890 FoundationStreamPusherSwiftWSStream (Starscream.swift)
7  PusherSwift                    0x101a3c21c specialized WebSocket.initStreamsWithData(Data, Int) -> () (Starscream.swift:987)
8  PusherSwift                    0x101a2c690 WebSocket.createHTTPRequest() -> () (Starscream.swift:868)
9  PusherSwift                    0x101a06918 PusherConnection.connect() -> () (PusherConnection.swift)
10 PusherSwift                    0x101a06aa0 @objc PusherConnection.connect() -> () (PusherConnection.swift)
11 Foundation                     0x184c545b8 __NSFireTimer + 88
12 CoreFoundation                 0x1841ffdc0 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 28
13 CoreFoundation                 0x1841ffae4 __CFRunLoopDoTimer + 864
14 CoreFoundation                 0x1841ff2e4 __CFRunLoopDoTimers + 248
15 CoreFoundation                 0x1841fcecc __CFRunLoopRun + 1928
16 CoreFoundation                 0x18411cc58 CFRunLoopRunSpecific + 436
17 GraphicsServices               0x185fc8f84 GSEventRunModal + 100
18 UIKit                          0x18d8755c4 UIApplicationMain + 236
19 --snip--
20 libdyld.dylib                  0x183c3c56c start + 4

and

EXC_BREAKPOINT
Crashed: com.vluxe.starscream.websocket
0  CoreFoundation                 0x183db52c0 CFHash + 360
1  CoreFoundation                 0x183db2160 CFBasicHashFindBucket + 164
2  CoreFoundation                 0x183db20a0 CFDictionaryGetValue + 224
3  CoreFoundation                 0x183e2b594 _CFStreamUnscheduleFromRunLoop + 184
4  CoreFoundation                 0x183ea9cc0 _CFStreamSetDispatchQueue + 168
5  PusherSwift                    0x102b20754 FoundationStream.cleanup() -> () (Starscream.swift:524)
6  PusherSwift                    0x102b21890 FoundationStreamPusherSwiftWSStream (Starscream.swift)
7  PusherSwift                    0x102b330e8 specialized WebSocket.disconnectStream(Error?, runDelegate : Bool) -> () (Starscream.swift:987)
8  PusherSwift                    0x102b25fcc WebSocketPusherSwiftWSStreamDelegate (Starscream.swift)
9  PusherSwift                    0x102b364e8 specialized FoundationStream.stream(Stream, handle : Stream.Event) -> () (Starscream.swift:568)
10 PusherSwift                    0x102b21348 @objc FoundationStream.stream(Stream, handle : Stream.Event) -> () (Starscream.swift)
11 CoreFoundation                 0x183e2b2d0 _signalEventSync + 212
12 CoreFoundation                 0x183eaa178 ___signalEventQueue_block_invoke + 24
13 libdispatch.dylib              0x183872a54 _dispatch_call_block_and_release + 24
14 libdispatch.dylib              0x183872a14 _dispatch_client_callout + 16
15 libdispatch.dylib              0x18387c96c _dispatch_queue_serial_drain$VARIANT$mp + 528
16 libdispatch.dylib              0x18387d2fc _dispatch_queue_invoke$VARIANT$mp + 340
17 libdispatch.dylib              0x18387dd20 _dispatch_root_queue_drain_deferred_wlh$VARIANT$mp + 404
18 libdispatch.dylib              0x18388603c _dispatch_workloop_worker_thread$VARIANT$mp + 644
19 libsystem_pthread.dylib        0x183b1af1c _pthread_wqthread + 932
20 libsystem_pthread.dylib        0x183b1ab6c start_wqthread + 4

Any improvements you suggest

I believe the code needs to have stronger logic in place to deal with concurrency (shuttle things to a serial queue and/or locking).


CC @pusher/mobile

@hamchapman
Copy link
Contributor

Hey @shawnce do you have any more contextual info you can provide about the crash?

For example:

  • what version of PusherSwift are you using?
  • how often / frequently does this happen?
  • what is the user / app doing when the crash happens (e.g. backgrounding the app, foregrounding the app, normal usage etc)?
  • what does the code look like where you're interacting with Pusher (e.g. disconnecting on view change, disconnecting on app being backgrounded, etc)?

@shawnce
Copy link
Author

shawnce commented Mar 2, 2018

  • Using PusherSwift (5.1.1)
  • It is the 3rd and 4th crash in terms of both number and number of users affected behind two unrelated of our own (one of which we just corrected). This app just released on phased rollout but if trends continue 100s of users will hit it a week.
  • It isn't clear yet what the user is doing but the pathway under com.vluxe.starscream.websocket kinda implies network connection was lost. The other crash pathway involves a timer firing, we don't have a pathway in our code using a timer when calling into pusher swift.
  • We setup pusher without any configuration other then our api key. We early in app launch bind to a couple of events on channels associated with one or more "projects'" that user has access to. This binding code will call subscribe as needed, then bind as needed, and finally calls connect if more then one binding exists. All of these calls happen from the main thread. We don't do any connect or disconnect after that, we leave it up to pusher. We also aren't unbinding unless the user logs out which is very very rare for someone to do with our app (all crashes show them logged in).

@shawnce
Copy link
Author

shawnce commented Mar 2, 2018

@hamchapman our layer of code calling into PusherSwift, again we are currently only calling into the bind function after app login to attach them to one or more "projects" and only calling unbind on logout which almost is never done (and all crashes show them logged in at the time).

https://gist.github.com/shawnce/63d701b6359fa98db986f0f9f2658bb5

@shawnce
Copy link
Author

shawnce commented Mar 6, 2018

@hamchapman we are continuing to see this crash

@mohammdsss1
Copy link

screen shot 2018-03-07 at 2 07 10 pm

@shawnce
Copy link
Author

shawnce commented Mar 8, 2018

FYI - we are disabling pusher given the crash rate we are seeing, if we find the time we may help with a patch

@hamchapman
Copy link
Contributor

Hey @shawnce that's understandable - apologies for the issue. Just wanted to let you know that I'm looking into this today. I started doing some groundwork to get things more stable and standardised yesterday with #175

I'll be sure to keep this updated if I find the cause. Equally, if you do have time to give to help debug then of course that's always massively appreciated!

@hamchapman
Copy link
Contributor

It looks as though this might be the cause of the issue: daltoniam/Starscream#476

I'm going to keep tracking that and see what comes of it. Will update here when I've got more info 👍

@shawnce
Copy link
Author

shawnce commented Mar 9, 2018

I am fairly sure you will want locking and/or shunting to a serial queue of your own... up higher in your code as wells since multiple thread can easily be involved in calling into your API. You will continue to have concurrent access/mutation to shared data otherwise which will crash in different ways.

@hamchapman
Copy link
Contributor

Yeah that's fair. I'll look into adding that extra layer of synchronisation later today.

@hamchapman
Copy link
Contributor

You should be able to try the cleanup-crash-fix branch (associated PR here: #178)

This should have fixed this problem.

Let me know if you're able to and what results you see 👍 I'm hoping to merge that PR and release 6.0.0 on Monday if all goes well.

@imann24
Copy link

imann24 commented Feb 9, 2019

Hi, thanks very much for releasing a fix. Entirely understand if it's impossible, but wanted to verify there's no way to backport this fix to 5.1 ?

Edit: just noticed the backport-6.0.0-fixes branch. May I inquire on the status?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants