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

Buffering bug fix. #9

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

ultinous-dancsa
Copy link

Hi,
There is a bug in the readline buffering. If multiple line buffered, only the first line is being consumed, subsequent calls to readline wont consume the buffer until more bufferable input provided as the buffer consuming happens only after the select() and on select timeout, the readline returns with nil.

Please consider merging this PR.

Thanks,
Daniel

@sonots
Copy link
Owner

sonots commented Jul 2, 2018

could you add tests?

@ultinous-dancsa
Copy link
Author

I don't know what tests do you expect to this trivial patch. The original readline() implementation has a bug, easily reproduced by
echo -e "1st line\n2nd line" > pipe
the second line wont be presented to the caller till the next write, as the select() will return no new data.
This is a serious bug, as messages can be lost, or seriously delayed. The pr contains our fix, that is tested in our system, it works, but we can't provide unit tests for this patch.

@sonots
Copy link
Owner

sonots commented Jul 18, 2018

Write the bad case you described as a unit test.
Without test, even if I merged this once, the plugin possibly can be broken after refactoring in the future.

@metacoma
Copy link

Hey folks, sorry about bump old thread, the master branch still have this issue, the code in Ultinous:master branch works fine.

metacoma added a commit to metacoma/mindwm that referenced this pull request Dec 20, 2022
metacoma added a commit to metacoma/mindwm that referenced this pull request Mar 9, 2023
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.

4 participants