-
Notifications
You must be signed in to change notification settings - Fork 844
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
Feature Request: completion handler for writes #1096
Comments
You're welcome to give it a shot at implementing this. |
When is this going to be released? |
Are there any updates on this? I see it's been merged into the develop branch, is there any way we can get a release for it? |
I hope to get a new version out this week. |
Awesome, thank you 👍 |
Note that this feature introduced major crashes. When reverting it (see https://github.com/vonox7/socket.io-client-swift/commits/v-revert-completion-handler ) the crashes are gone. Crashed: NSOperationQueue 0x280ab42e0 (QOS: UNSPECIFIED) 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 ( Related issue: #1118 |
See comment on #1118 as I haven't been able to reproduce this. |
We've hit a case where we need to know when a write to the underlying socket has been completed. We're sending quite a large amount of latency sensitive data, such that the network can sometimes not keep up. It would be really helpful to be able to monitor when a write actually completes so we can scale back writes when the network slows down. The underlying Starscream WebSokcet class already provides this functionality via a completion handler called on write completion. Ideally this would be exposed up through socket.io as well.
We've considered using the ack mechanism from the server that's already supported by this library, but the extra latency that the network call introduces is unacceptable for our use case.
Is there a way to achieve this with the current APIs? And if not would you accept a PR adding this functionality?
The text was updated successfully, but these errors were encountered: