-
Notifications
You must be signed in to change notification settings - Fork 65
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
Maintain callback queues accordingly #9
Conversation
@eventengineering, thanks for the PR, it's really helpful 👍 @aprock, can you review this PR and merge it ASAP please? This fix will benefit for scenario where we want to send multiple write from server to client. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi all, I help to maintain this library. I have to admit, I'm having a hard time evaluating this PR, especially without tests on this library.
@eventengineering @richardtks are either you running this code in an app and can verify that it works as intended?
This is my understanding of the change: methods that were defined on TcpSockClient are being moved to TcpSockets so that they can be used externally to the library. Is that accurate?
@@ -31,6 +31,10 @@ typedef enum RCTTCPError RCTTCPError; | |||
- (void)onData:(NSNumber *)clientID data:(NSData *)data; | |||
- (void)onClose:(TcpSocketClient*)client withError:(NSError *)err; | |||
- (void)onError:(TcpSocketClient*)client withError:(NSError *)err; | |||
- (NSNumber*)getNextTag; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this can be added to the public interface as the definitions have been deleted below. Are these supposed to be public in the TcpSockets.h?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@beau6183 might know - this code all works for me, but then, it might not be based off the latest changes.
@beau6183 I believe you submitted the original PR, can you help explain the logic again? |
The purpose of this PR is to make sure callbacks go back to the correct place. Prior to this commit, multiple instances of TCPSocket clashed and caused issues. This PR moves the functionality to the appropriate places as to allow multiple instances to run concurrently and correctly. |
Honestly, it's been a year+ since I've looked at my PR I don't recall the details around it, but @eventengineering has the gist of it. |
Just wanted to chime in and say that I had to make a fork of this repo and pull these changes in so that this repo is usable on iOS, otherwise the server can only ever send a single packet then it gets deadlocked. I've been using it without issues for quite a while now. Unrelated to this PR I have had some issues with this library that I'd love to get some eyes on but there's no issues tab on this repo. What's the appropriate way of sending feedback to the maintainers? |
Port of PeelTechnologies#87