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

PusherConnection delegate crash #109

Closed
BasThomas opened this issue Nov 16, 2016 · 21 comments
Closed

PusherConnection delegate crash #109

BasThomas opened this issue Nov 16, 2016 · 21 comments

Comments

@BasThomas
Copy link
Contributor

BasThomas commented Nov 16, 2016

Hi, when following the standard getting started, calling pusher.connect() crashes on line 42 in PusherConnection.swift. Specifically, the line shown here crashes.

self.delegate?.debugLog?(message: "[PUSHER DEBUG] Network reachable")

I can track it back as far as line 171 in Reachability.swift:

self.reachabilityChanged()

My code:

let pusher = Pusher(key: PusherSwift.key, options: PusherClientOptions(host: .cluster(PusherSwift.cluster)))
let channel = pusher.subscribe("session-status-1")

let _ = channel.bind(eventName: "App\\Events\\SessionStatusUpdated") {
  guard let data = $0 as? [String: Any], let message = data["message"] as? String else { return }
  print(message)
}

pusher.connect()

My PusherSwift.swift:

enum PusherSwift {
  
  static var key: String {
    return "xxx"
  }
  
  static var cluster: String {
    return "eu"
  }
}

Any idea?

@hamchapman
Copy link
Contributor

Do you have a stack trace or error message?

@BasThomas
Copy link
Contributor Author

BasThomas commented Nov 16, 2016

Does this help?

(lldb) bt
* thread #8: tid = 0x8f3d1e, 0x00000001039c349f libswiftCore.dylib`_swift_abortRetainUnowned + 15, queue = 'uk.co.ashleymills.reachability', stop reason = EXC_BREAKPOINT (code=EXC_I386_BPT, subcode=0x0)
  * frame #0: 0x00000001039c349f libswiftCore.dylib`_swift_abortRetainUnowned + 15
    frame #1: 0x00000001039d6ac1 libswiftCore.dylib`swift_unknownUnownedLoadStrong + 65
    frame #2: 0x0000000100b250ae PusherSwift`PusherConnection.(reachability=0x000060000009b300, self=0x000060000000d421).(closure #1).(closure #1) + 46 at PusherConnection.swift:42
    frame #3: 0x0000000100b25477 PusherSwift`thunk + 23 at PusherConnection.swift:0
    frame #4: 0x0000000100b27121 PusherSwift`partial apply for thunk + 81 at PusherConnection.swift:0
    frame #5: 0x0000000100b36d53 PusherSwift`Reachability.reachabilityChanged(self=0x000060000009b300) -> () + 307 at Reachability.swift:246
    frame #6: 0x0000000100b38535 PusherSwift`Reachability.(self=0x000060000009b300) throws -> ()).(closure #1) + 21 at Reachability.swift:171
    frame #7: 0x0000000100b1bd87 PusherSwift`thunk + 39 at PusherConnection.swift:0
    frame #8: 0x000000010515e980 libdispatch.dylib`_dispatch_call_block_and_release + 12
    frame #9: 0x00000001051880cd libdispatch.dylib`_dispatch_client_callout + 8
    frame #10: 0x0000000105165e6b libdispatch.dylib`_dispatch_queue_serial_drain + 236
    frame #11: 0x0000000105166b9f libdispatch.dylib`_dispatch_queue_invoke + 1073
    frame #12: 0x000000010516707f libdispatch.dylib`_dispatch_queue_override_invoke + 683
    frame #13: 0x00000001051693b7 libdispatch.dylib`_dispatch_root_queue_drain + 720
    frame #14: 0x000000010516908b libdispatch.dylib`_dispatch_worker_thread3 + 123
    frame #15: 0x0000000105537736 libsystem_pthread.dylib`_pthread_wqthread + 1299
    frame #16: 0x0000000105537211 libsystem_pthread.dylib`start_wqthread + 13
(lldb) 

@hamchapman
Copy link
Contributor

Hmmm, are you setting a breakpoint anywhere?

Also, where is that code that you pasted being run? In a ViewController?

Do you have a sample project / app that you could provide that exhibits the problem?

Thanks!

@BasThomas
Copy link
Contributor Author

No, not using any breakpoints. The code runs in a ViewController, indeed. Not working from both viewDidLoad and viewDidAppear. Let me post a sample project in a sec!

@BasThomas
Copy link
Contributor Author

Hmm, can't reproduce in an empty project with the code used in the full project. Let me investigate further.

@BasThomas
Copy link
Contributor Author

Strange - I can't seem to replicate this in an empty project. Let me close this for now and investigate further. Will reopen when I find something.

@ay8s
Copy link

ay8s commented Nov 21, 2016

Seeing this one also, curious if you figured out the cause @BasThomas?

@BasThomas
Copy link
Contributor Author

Haven't had time to look into it yet. Will update when I find something.

@hamchapman
Copy link
Contributor

@ay8s do you have a sample app that reproduces this?

Are you getting the same error / stack trace?

Or if easier, could you share the code where you're doing Pusher-related things?

This sounds like a nasty crash so I'm keen to dig into it and fix it!

@hamchapman hamchapman reopened this Nov 21, 2016
@BasThomas
Copy link
Contributor Author

It's crashing in an empty project for me now as well. I'll create a repo.

@BasThomas
Copy link
Contributor Author

https://github.com/BasThomas/PusherSwiftCrash

@hamchapman
Copy link
Contributor

Thanks for the project - just been having a play and I think it's that the PusherConnection object is taken as unowned into the reachability closure but because you're not keeping a reference to the Pusher instance outside of the viewDidLoad function then the connection object gets cleaned up whereas the reachability object does not.

I'm not sure what the best solution is here.

Perhaps self could be weak instead of unowned?

Or maybe it's a question of making the expected usage clearer in the docs. In your example project I get the crash but even if you were to comment out all of the reachability code then I don't think the Pusher interactions would work as expected seeing as the objects would be being cleaned up as soon as the view loads.

WDYT?

@BasThomas
Copy link
Contributor Author

Hmm, I see what the problem is. IIRC this was a problem with Apple's CLLocationManager as well.

I am not sure what the difference between weak and unowned is under the hood - but I do think the deallocation should be prevented on your end; the user shouldn't be forced to create a Pusher instance at file scope.

@BasThomas
Copy link
Contributor Author

So @ay8s, if this is vague for you - if you declare:

var pusher: Pusher!

in your ViewController, then initialize it in you function, it should work.

@hamchapman
Copy link
Contributor

As I understand it, unowned means that you're sure that the object that you're saying is unowned will not be nil when you reference it inside the closure, whereas weak means that it might be. It's a bit like using ! instead of ?. Using weak would mean that you wouldn't get the crash but it still means (I'm pretty sure) that the rest of the Pusher-related objects would get cleaned up.

I'm not sure if there's a way to stop the deallocation of the Pusher object when it's not referenced outside the scope of the viewDidLoad function. Even if there is, then I imagine it might lead to the opposite problem, where if someone already has a reference to it outside of the viewDidLoad function then you might end up with the retain count being 1 too high for those use cases.

And thanks for the code snippet explaining what I meant!

@hamchapman
Copy link
Contributor

Would still be good to see what your setup is @ay8s, if you're cool with that

@BasThomas
Copy link
Contributor Author

Right. Tricky issue. I think it'd be good to explicitly tell developers in the docs and code snippets, and explain why it is necessary.

@hamchapman
Copy link
Contributor

hamchapman commented Nov 21, 2016

I think I'll likely do a couple of things:

  1. Make the code snippets / readme clearer about the need to retain the Pusher object
  2. Change the unowned self in the capture list to weak self and then potentially log something out suggesting that the PusherConnection object (and therefore likely the Pusher object) has been deallocated when it shouldn't have been if it's nil when it's referenced in the Reachability closure.

And like I said, I'll wait to see if Andy has anything to add before closing this.

Thanks for the input @BasThomas!

@BasThomas
Copy link
Contributor Author

Yep, that seems like a great idea! And you're welcome on the input 🙂

@hamchapman
Copy link
Contributor

hamchapman commented Jan 23, 2017

I've gotten round to doing what I described above, at least in terms of code. Still going to make README additions

@patriksharma
Copy link

patriksharma commented May 3, 2019

Hey I'm facing this strange issue.

All works fine when app is in foreground. As soon as I put app in background and Lock the phone.

I see in Xcode logs I'm getting this error message with no Push notification for messages.
Your Pusher instance has probably become deallocated. See https://github.com/pusher/pusher-websocket-swift/issues/109

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

No branches or pull requests

4 participants