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
curl_easy_recv(): Handle end of stream (EOS) properly #1134
Conversation
@mkauf, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bagder, @yangtse and @dfandrich to be potential reviewers. |
Ref #1129 (comment) We could probably do for a better adjustment in curl_easy_recv, I think the existing published wording is misleading:
The problem as we recently discussed is for example that we have SSL libraries that will receive data in records, so we decrypt the record and hold the result of the data (or the SSL lib does it for us) if it was more than what was asked for. I suspect (but I haven't reviewed the connect_only code paths to prove it) that in those cases waiting on the socket could just end up waiting I think it may be better to encourage the user to call curl_easy_recv until it returns CURLE_AGAIN, and repackage what you said about the SSL like this:
|
@mkauf, what's the benefit of this change? maybe some apps rely on current behaviour (return value).
@jay, I think usually when you get all the bytes you asked for, then you consider the socket still readable. |
@Frenche According to the current documentation, the current implementation is wrong (or vice versa). I think the documentation makes more sense. Currently
Expert programmers will probably do this, but it's easier to explain to "just call @jay, thank you for the suggestion! I think that's a good idea. I will include it in an update of this pull request. |
@mkauf right, I didn't realize the doc says you get zero bytes when the connection is closed. I wonder why we need to peek at the connection in Curl_getconnectinfo(), can't we let subsequent recv call to find out the status? it looks a bit strange, especially when it is invoked from curl_easy_getinfo(). |
Because we want to avoid sending back a socket to an already dead connection and that is one way to figure that out. We can of course debate the value in preventing that, because as you say the following recv() on that socket would return error anyway. |
Return CURLE_OK and set *n to 0 on EOS. Fix the example program sendrecv.c (handle CURLE_AGAIN, handle incomplete send) and improve the documentation for curl_easy_recv() and curl_easy_send(). Reviewed-by: Frank Meier Assisted-by: Jay Satiro
37386ea
to
8b03a4e
Compare
I have updated the pull request and included @jay's suggestion. |
I see, but anyway we cannot guarantee the socket would still be valid when the app calls curl_easy_recv(), as the peer might reset it in between, no? The reason I'm debating it here, is because I think @mkauf's patch could be greatly simplified if we get rid of this check (and also because I think curl_easy_getinfo() shouldn't do any socket operations). I've looked up Curl_getconnectinfo() and it is invoked in four places.
Also, in the case where it is invoked from curl_easy_send(), I think the check is conceptually wrong, because in theory (imaginary protocol, not HTTP) the peer might have called shutdown(SHUT_WR) to close its write-end (our read-end) but still expect to read data from us. |
@@ -49,3 +49,4 @@ Added in 7.15.2 | |||
Returns CURLE_OK if the option is supported, and CURLE_UNKNOWN_OPTION if not. | |||
.SH "SEE ALSO" | |||
.BR CURLOPT_VERBOSE "(3), " CURLOPT_HTTPPROXYTUNNEL "(3), " | |||
.BR curl_easy_receive "(3), " curl_easy_send "(3) " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be curl_easy_recv not receive, it should fix test 1140 failure.
Please take a look at: https://github.com/frenche/curl/tree/avoid_socket_check |
@Frenche Thank you, your commit is alright! I have made some cosmetic changes and pushed it, together with a second commit to improve the example program and the documentation. |
Great, thanks! |
Return CURLE_OK and set *n to 0 on EOS. Fix the example program sendrecv.c
(handle CURLE_AGAIN, handle incomplete send) and improve the documentation for
curl_easy_recv() and curl_easy_send().
Reviewed-by: Frank Meier