Optional write completion handler for emit's cause major crashes

[vonox7]

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:

Crashed: NSOperationQueue 0x280ab42e0 (QOS: UNSPECIFIED)
0  libobjc.A.dylib                0x1d853cd70 objc_msgSend + 16
1  Starscream                     0x1050ff6a8 protocol witness for WSStream.write(data:) in conformance FoundationStream (WebSocket.swift:241)
2  Starscream                     0x105105c30 closure #1 in WebSocket.dequeueWrite(_:code:writeCompletion:) (WebSocket.swift:1268)
3  Starscream                     0x1050fd244 thunk for @escaping @callee_guaranteed () -> () (<compiler-generated>)
4  Foundation                     0x1d9de3b6c __NSBLOCKOPERATION_IS_CALLING_OUT_TO_A_BLOCK__ + 16
5  Foundation                     0x1d9cebcc8 -[NSBlockOperation main] + 72
6  Foundation                     0x1d9ceb19c -[__NSOperationInternal _start:] + 740
7  Foundation                     0x1d9de5a40 __NSOQSchedule_f + 272
8  libdispatch.dylib              0x1d8d8f6c8 _dispatch_call_block_and_release + 24
9  libdispatch.dylib              0x1d8d90484 _dispatch_client_callout + 16
10 libdispatch.dylib              0x1d8d66e14 _dispatch_continuation_pop$VARIANT$armv81 + 404
11 libdispatch.dylib              0x1d8d664f8 _dispatch_async_redirect_invoke + 592
12 libdispatch.dylib              0x1d8d72afc _dispatch_root_queue_drain + 344
13 libdispatch.dylib              0x1d8d7335c _dispatch_worker_thread2 + 116
14 libsystem_pthread.dylib        0x1d8f72190 _pthread_wqthread + 472
15 libsystem_pthread.dylib        0x1d8f74d00 start_wqthread + 4

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 was nil. This a) introduces a major performance hit and b) apparently is somehow not 100% save which leads to a lot of crashes in production.

[headlessme]

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

[nuclearace]

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.

[headlessme]

Ok good to know - thanks @nuclearace

[vonox7]

@headlessme I also can't recreate it, just when I deploy the library to production the crashes occour.
There are many occurences that have this faulty pattern:

1. https://github.com/socketio/socket.io-client-swift/pull/1097/files#diff-0a1f28547352834d5f60fc457346f374R216: {} gets created and passed.

2. https://github.com/socketio/socket.io-client-swift/pull/1097/files#diff-0a1f28547352834d5f60fc457346f374R233: completion should be optional and default to nil

3. https://github.com/socketio/socket.io-client-swift/pull/1097/files#diff-0a1f28547352834d5f60fc457346f374R250: {} gets created and passed.

4. https://github.com/socketio/socket.io-client-swift/pull/1097/files#diff-0a1f28547352834d5f60fc457346f374R318: wrappedCompletion closure gets created, even when not used

5. https://github.com/socketio/socket.io-client-swift/pull/1097/files#diff-82f151b5e75e68a03348f7b49ecc92bdR112: completion should be optional and default to nil

6. https://github.com/socketio/socket.io-client-swift/pull/1097/files#diff-f4c64640e3bdff245461736366db089eR343: {} gets created and passed.

7. https://github.com/socketio/socket.io-client-swift/pull/1097/files#diff-f4c64640e3bdff245461736366db089eR351: {} gets created and passed.

8. https://github.com/socketio/socket.io-client-swift/pull/1097/files#diff-f4c64640e3bdff245461736366db089eR369: {} gets created and passed.

9. https://github.com/socketio/socket.io-client-swift/pull/1097/files#diff-f4c64640e3bdff245461736366db089eR547: {} gets created and passed.

10. https://github.com/socketio/socket.io-client-swift/pull/1097/files#diff-f4c64640e3bdff245461736366db089eR614: completion should be optional and default to nil

11. https://github.com/socketio/socket.io-client-swift/pull/1097/files#diff-68876de916c9842709454984f828e6d9R68: completion should be optional and default to nil

12. https://github.com/socketio/socket.io-client-swift/pull/1097/files#diff-68876de916c9842709454984f828e6d9R216: completion should be optional and default to nil

13. https://github.com/socketio/socket.io-client-swift/pull/1097/files#diff-05aa04316266fce09c354930d7e392ebR141: completion should be optional and default to nil

14. https://github.com/socketio/socket.io-client-swift/pull/1097/files#diff-05aa04316266fce09c354930d7e392ebR184: if completion is nil, we should also pass nil to the completion param.

15. https://github.com/socketio/socket.io-client-swift/pull/1097/files#diff-d92ca8e6cf38ee3f5cce2f6aa0995b07R41: completion should be optional and default to nil

16. https://github.com/socketio/socket.io-client-swift/pull/1097/files#diff-d92ca8e6cf38ee3f5cce2f6aa0995b07R60: completion should be optional and default to nil

17. https://github.com/socketio/socket.io-client-swift/pull/1097/files#diff-3f5960a15123f6282c649b10d18adf28R293: {} gets created and passed.

18. https://github.com/socketio/socket.io-client-swift/pull/1097/files#diff-1c4c08a7f3a6b280a37a22d512291f7aR191: completion should be optional and default to nil

I'm not sure if I found all faulty occurences, but this bad pattern is almost everywhere.

[vonox7]

@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?

[nuclearace]

@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.

[headlessme]

@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 😄

[nuclearace]

@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.

[nuclearace]

Also, that stack trace looks like it ends in a Starscream path that doesn't even interact with this feature?

[vonox7]

@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?

[nuclearace]

@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.

[vonox7]

@nuclearace yes, I'm currently making a PR that propagates nil.