-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Repeatedly divide read buffer size by 8 until success. Fixes #11481. #18332
Conversation
d7b45c3
to
e081221
Compare
Tests passed for me locally (64-bit Win7). Looks like the AppVeyor failure was due to some network issue on their end? |
LGTM. Possible to test? Maybe add a comment too? |
I'll add a comment and a test. |
2e6ef14
to
8d55e39
Compare
…g#11481. Dividing by 8 leads to success on the second try in the case of the failure of --lisp on Windows 7.
8d55e39
to
0440507
Compare
I have been unable to construct an automated test case for this. One issue is that I can't duplicate the error if julia is run in mintty, even if I run an inner instance of julia through cmd. For instance:
results in a functional femtolisp, even for a julia without this patch, so the problem just doesn't exist if there is a mintty somewhere in the parentage. Since the testbed is almost certainly always going to be run from a cygwin/msys2 terminal, this looks like a deal killer. Further, if input is piped in to femtolisp, it works OK, without the patch. For instance, I was considering doing something like
and testing the return code. If I got 42, then the lisp worked. If it wasn't able to process the input and returned immediately, I'd get some other return code (presumably). Unfortunately, this command actually works for the unpatched julia. In sum, the bug only manifests if:
So automating a test case might require more cleverness than I can muster. |
fair enough. appveyor and most people who would run |
This solution fixes
julia --lisp
on Windows 7 by repeatedlyhalvingdividing by 8 the number of bytes requested on a failed read and trying again, implementing the suggestion of @vtjnash in #11481.The behavior should be unchanged on Windows 10 (or other platforms for which there wasn't previously a failure).