-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Addresses #24291 #24452
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
Addresses #24291 #24452
Conversation
Connected to Huly®: V_0.6-22822 |
If you do not know what the cause of the issue is, and can not make a test, how are you sure, that you have fixed it, and that your changes would not regress or cause problems for others? |
I have investigated this issue. The way I discovered it is by talking to a server that disconnects unexpectedly. When the server disconnects, you would expect Eof to be returned when attempting to read, instead I got Unexpected. |
I do not want to merge it without a test, since this part of the code is brittle and already had a period of instability, where different people wanted different behaviors. See the existing |
I agree, however, I do not know when I'll have time to write a test. |
@einar-hjortdal do you have a way to test it now? |
No, I'm sorry, I do not have time to work on this. |
Thanks for the PR, but without a test, I will not merge it unfortunately, and since you do not have time, its status is unlikely to change. Hopefully in the future, there will be an opportunity to find a more reproducible way to reveal the problem, if it exists, and add a fix + test for it. I am closing it for now. |
The problem does exist; it is clear if you read the source code. What matters is that users are informed that the |
I've read the source code and your patch, and I could not infer a test that would reproduce the problem that you saw. To me, that means that the problem is not clear, or I would have added the test myself a long time ago, and this would not have waited in a limbo for 2 months. -> I wanted a test from you, in order to prevent future regressions, as I already stated above. If you do not have time to make it, and if there is not some other developer that can, that understands it better than me, and has more time than you, I do think it is better, to have this closed, and wait until there is a better reproduction/fix. |
Just to be clear, the issue is here: |
ok, hopefully that explanation, clears it up for the developer that will write the test, and we could then reopen the PR or make a new one. |
Seems to solve the issue, can't find negatives.
I am not the author of BufferedReader, I do not know what the purpose of the check was.
Adding a test to prevent a regression would be nice, but I do not know what causes the issue in the first place.