Repository navigation
telnet: exit immediately when stdin reaches EOF - #23057
JayRGadekar wants to merge 1 commit into
Conversation
read() returns 0 at EOF (e.g. when stdin is redirected from /dev/null or a finite file), but telnet_do() only checked for a positive byte count (send data) or a negative one (stop reading), so EOF was silently ignored and the loop kept polling until curl's overall timeout expired instead of exiting right away. The fix needs to live in both stdin-reading paths in telnet_do(): the direct read() used when polling stdin's fd, and the fread_func callback path, which the curl command-line tool actually goes through by default. A fix in only the first branch, which is the more obvious one, would look correct but not actually affect the curl tool at all. Adds test 5029, which feeds telnet a finite stdin and checks that curl exits 0, exercising the fread_func callback path directly. See docs/TODO's TELNET section, "exit immediately upon connection if stdin is /dev/null" (no existing tracking issue).
|
Analysis of PR #23057 at 68e8cd49: Test 1452 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 30 different CI jobs (the link just goes to one of them). Note that this CI job has had a number of other flaky tests recently (2, to be specific) so it may be that this failure is rather a systemic issue with this job and not with this specific PR. Test 1548 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 178 different CI jobs (the link just goes to one of them). Note that this CI job has had a number of other flaky tests recently (2, to be specific) so it may be that this failure is rather a systemic issue with this job and not with this specific PR. Test 1714 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 176 different CI jobs (the link just goes to one of them). Note that this CI job has had a number of other flaky tests recently (2, to be specific) so it may be that this failure is rather a systemic issue with this job and not with this specific PR. Test 3555 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 62 different CI jobs (the link just goes to one of them). Note that this CI job has had a number of other flaky tests recently (2, to be specific) so it may be that this failure is rather a systemic issue with this job and not with this specific PR. Test 1326 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 22 different CI jobs (the link just goes to one of them). Note that this CI job has had a number of other flaky tests recently (3, to be specific) so it may be that this failure is rather a systemic issue with this job and not with this specific PR. Test 3232 failed, but it has been 82.2% flaky lately, so it's probably NOT a fault of the PR. Note that this CI job has had a number of other flaky tests recently (2, to be specific) so it may be that this failure is rather a systemic issue with this job and not with this specific PR. Generated by Testclutch |
|
Thanks for the CI run — I want to flag something important before this goes The failures aren't incidental. Looking at test 1548 specifically: That test runs with So this isn't a narrow edge case my fix missed — it's a real conflict Given that, I don't think this should be a change to curl's default Happy to take a pass at an opt-in version if that sounds like the right |
When you fix something in curl, it is not uncommon that several existing tests need updates that verified the previous behavior. Like test 1548 that looks wrong as it does read a EOF on stdin contrary to the intention. I'll work on fixing that test in a separate PR: #23081.
I disagree. What should curl do when the EOF has been received other than stopping the transfer and why would any user want that? |
|
It seems easiest to change the expectation of test1548 to a curl exit with 0. @bagder, thoughts? |
|
Hm, the answer would of course be: what if the stdin ends while there is data still arriving over the network. That could certainly be a use case. |
I thought about that. Unfortunately, it is impossible to know when the data from a telnet server will end. |
|
Thanks both for digging into this, and for opening #23081 to fix the test — that's really helpful. @bagder, it seems there's no way to know when a telnet server is truly done sending, so waiting indefinitely isn't really "correct" either — it's just a longer arbitrary cutoff. Not knowing when data ends seems like an argument for uncertainty, not necessarily one that favors stopping early over continuing. So I'd like to ask directly: is cutting off that trailing data a trade-off you're comfortable accepting as the new default? Full transparency since this is my first open source PR: I wasn't aware that fixing behavior in curl can mean existing tests that So my main question is whether we should go forward with this PR. as both decisions have their own tradeoffs being either connection being open indefinitely or connection closing the moment local input runs out, which risks cutting off server data still in flight. |
This reverts commit d745b13. Setting stdin as non-blocking unconditionally caused it to affect future shell commands. There is probably a way to fix it but I think this needs a rethink. For now we revert. Ref: #23057 Ref: #23081 Reported-by: Daniel S. Roche Researched-by: KayanoLiam@users.noreply.github.com Fixes #23127 Closes #23144
4 participants
read() returns 0 at EOF (e.g. when stdin is redirected from /dev/null or a finite file), but telnet_do() only checked for a positive byte count (send data) or a negative one (stop reading), so EOF was silently ignored and the loop kept polling until curl's overall timeout expired instead of exiting right away.
The fix needs to live in both stdin-reading paths in telnet_do(): the direct read() used when polling stdin's fd, and the fread_func callback path, which the curl command-line tool actually goes through by default. A fix in only the first branch, which is the more obvious one, would look correct but not actually affect the curl tool at all.
Adds test 5029, which feeds telnet a finite stdin and checks that curl exits 0, exercising the fread_func callback path directly.
See docs/TODO's TELNET section, "exit immediately upon connection if stdin is /dev/null" (no existing tracking issue).