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

Fix #2: Change read logic #5

Merged
merged 1 commit into from
Mar 11, 2019

Conversation

matracine
Copy link
Collaborator

Hello,
The problem was in Client's read() method logic. I leave comments in source code explaining how mikrotik API sends replies.
The problems only appears when you send multiples messages in the same connexion.

  • The affectation of $isDone was executed on each read loop, it is true only when you read the !done line, false the rest of time. $isDone must be a one time change flag that is raised when you read the !done (or !fatal) value.
  • The test to break the read loop is wrong. : if (A or (A and B)) is same as if (A). So value of B doesnot matters. ($isDone in this case), wich is a good thing because its value is wrong due to precedent point
  • your A value was 'my TCP queue is empty', you must not rely on this type of information when implementing a TCP communication protocol : if the link is slow or the router much less powerfull than the server that read data, you can consume the datas faster than they arrive. This information is too low level, a protocol relying on TCP must not, because it can not, rely on this.

I reproduced the bug with a loop on 10 consecutives requests, tested the new read method with a loop of 200 results, works OK.

Regards.

@EvilFreelancer
Copy link
Owner

@matracine hello! Cool update, thank you very much 👍 let me check how it work on real routerboard.

@EvilFreelancer
Copy link
Owner

I've reproduced the bug by 100 requests (all after first was with errors), and tested your solution by 1000 requests and all was fine. So thanks you @matracine again for your awesome ideas :) Btw, code is covered and not need write any additional tests. Update with your fix will released with 0.8.2 version tag

@EvilFreelancer EvilFreelancer merged commit 00a84b7 into EvilFreelancer:master Mar 11, 2019
@EvilFreelancer
Copy link
Owner

@matracine I've sent to you invite with write access to my repo, so you can push your updates directly, without forks.Well done :)

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.

2 participants