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

Maintain callback queues accordingly #9

Merged
merged 1 commit into from
Apr 9, 2020

Conversation

eventengineering
Copy link

@richardtks
Copy link

richardtks commented Nov 14, 2019

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

Copy link
Collaborator

@phillbaker phillbaker left a 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;
Copy link
Collaborator

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?

Copy link
Author

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.

@phillbaker
Copy link
Collaborator

@beau6183 I believe you submitted the original PR, can you help explain the logic again?

@eventengineering
Copy link
Author

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.

@beau6183
Copy link

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.

@acb
Copy link

acb commented Apr 9, 2020

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?

@aprock aprock merged commit 356e18c into aprock:master Apr 9, 2020
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

Successfully merging this pull request may close these issues.

7 participants