Optional write completion handler for emit's cause major crashes #1118
Comments
|
I can't recreate this - which library versions and under what conditions do you see this? The PR I created only passed through the wrapped version of the completion handler if it was not nil: https://github.com/socketio/socket.io-client-swift/pull/1097/files#diff-0a1f28547352834d5f60fc457346f374R334 |
|
That stack trace doesn't look related to this work. It looks like a crash that occasionally happens in Starscream. Do you have a way to reproduce the crashes? Without that the stacktrace isn't very helpful. @headlessme I believe I got rid of the optional bit because it shouldn't have a noticeable impact passing an empty closure when you'll inevitably still create one down the line. |
|
Ok good to know - thanks @nuclearace |
|
@nuclearace Then how can you explain why commit 4ce1e92 on https://github.com/vonox7/socket.io-client-swift/commits/v-revert-completion-handler fixes this issue to 100%, and without it i got hundreds of crashes from various users? |
|
@vonox7 I can certainly look into the crashes. But this hasn't even been released yet, so if you're using the development branch in a production app, it's on you for releasing an unstable app. The development branch is not meant to be production-stable code. |
|
@vonox7 I'm sure a PR would be welcomed to resolve any areas you think need improvement before this feature gets released – GitHub is all about collaboration after all 😄 |
|
@vonox7 I'm not entirely sure how it could be related yet if they are. This will be hard to debug without a reproducible case. |
|
Also, that stack trace looks like it ends in a Starscream path that doesn't even interact with this feature? |
|
@nuclearace All I can say that after reverting this PR in my branch https://github.com/vonox7/socket.io-client-swift/commits/v-revert-completion-handler, this issue was resolved. I don't say that this isn't also an issue on Starscream. But do you see why this is a big performance hit and why reverting this PR also resolves the issue? |
|
@vonox7 Actually going through this code again, because of that final dispatch queue stuff that it does, it probably is better to make sure we propagate nil when no closure is given. I don't see how it could have caused huge performance problems and crashes unless you were sending a ton of messages in a very short amount of time. Did you want to PR that change? It is usually faster to make the change yourself and open a PR that I can review. |
|
@nuclearace yes, I'm currently making a PR that propagates |
|
And yes, I make a ton of message, thats why I use sockets and not a simple http connection. PR: #1119 |
|
@vonox7 your PR has been merged |
* development: Remove unnecessary function calls. Fix build Don’t create empty closures when we don’t need them. Fixes #1118. Propagate http header callback from websocket back to client. clean up emit completion code update cartfile too update deps: Fix #1105 update xcode settings convert SocketData to serialisable socket representation Add a variadic method for emit completion handlers in swift no need to capture completion hander in wrapper wrap completion handler to run on handleQueue add optional write completion handler for emit's
|
@nuclearace for me this issue is still happening in production (on v15.1.0), though it is rather rare. |
PR #1097 creates major crashes in production. When reverting the PR (see https://github.com/vonox7/socket.io-client-swift/commits/v-revert-completion-handler ) the crashes are gone.
Crashed: NSOperationQueue 0x280ab42e0 (QOS: UNSPECIFIED)
EXC_BAD_ACCESS KERN_INVALID_ADDRESS 0x0000000a35918130
Stacktrace:
I think, that the main cause of this issue is that @headlessme introduced for every single call a requestHandler-closure, even when it is empty (
{}). Before this commits, it wasnil. This a) introduces a major performance hit and b) apparently is somehow not 100% save which leads to a lot of crashes in production.The text was updated successfully, but these errors were encountered: