Conversation
188f3d6 to
b1b3718
Compare
| socket. This can surface as zero-length reads or spurious data when | ||
| the connection is later closed or reused. Applications using | ||
| CURLMOPT_SOCKETFUNCTION(3) should handle such events without assuming | ||
| the socket is still actively used by libcurl. |
There was a problem hiding this comment.
This is slightly inverted.
While idle connections are not supposed to receive data (according to http specs), they can receive zero length messages indicating connection closed by server. (non-zero length messages when using https, that decrypt to zero-length message)
However CURL_POLL_REMOVE has told us not to listen for read events.
We can't send CURL_CSELECT_ERR to libcurl in those cases because idle connections don't get handled in curl_multi_socket_action.
So we can't cheat by listening to read events for idle connections for three reasons:
- There a way to get libcurl to close an fd in idle connection pool.
- We can't peek at the message to check if it is zero-length. (http)
- We can't decrypt the message to determine it is zero-length. (https)
Also point out that if a socket is NOT in read nor write, and is closed by the kernel for some reason, that fd can be reused and cause conflicts with libcurl's internally managed list of fds.
There was a problem hiding this comment.
Thanks for the clarification that makes sense.
You're right: the wording is slightly inverted. The intent was not to suggest that applications should listen for read events on idle connections, but rather to document why they may still surface and why libcurl cannot safely act on them once CURL_POLL_REMOVE has been issued.
I’ll update the text to emphasize:
- That idle connections are not expected to receive application data
- That zero-length reads (or encrypted data decrypting to zero-length) can still occur on close
- That libcurl cannot inspect, decrypt, or act on such events for idle connections
- And that fd reuse after kernel close is the underlying risk applications need to account for
I’ll push a small doc fix shortly.
bd79658 to
6b1fd51
Compare
|
Looks like the Alpine musl + c-ares DNS flake (test 2103). |
|
Analysis of PR #20521 at 6b1fd514: Test 2103 failed, which has NOT been flaky recently, so there could be a real issue in this PR. Note that this test has failed in 12 different CI jobs (the link just goes to one of them). Generated by Testclutch |
|
@testclutch Thanks for the heads-up. |
|
I’ve updated the documentation to align more closely with the intended contract and wording expectations. Changes in this revision: Please let me know if this better matches the documentation style you’re aiming for, or if further tightening is needed. |
|
Thanks for the detailed review as that helped clarify the scope here. I’ve updated the PR to: Any remaining questions about idle connection observability probably belong in the socket function docs or a separate design discussion, so I’ve kept this PR narrowly scoped. |
8a5a1cd to
c2c8195
Compare
|
Hi @bagder I hate to be the bearer of bad news. But Jayvenn21 is using some sort of continuous AI agent to interact on Github. I highly doubt there is a human reading any of the extensive review comments that you have left :( More likely your comments are being fed directly into an AI chatbot which is generating and pushing further patches. Since you have left such extensive reviews, maybe the final result is fine, but I would hate to see you waste further time trying to give nice and helpful feedback to an AI chat bot who will not learn from this interaction and will never become a future contributor. We just banned this user from the Godot repo after they replied to our admonition for submitting non-functional AI-generated code with an AI-generated (non)apology. Example here: godotengine/godot#116056 |
|
Hi @clayjohn fyi and just to clarify, I’m actively reading and responding to review feedback myself. I do use tooling to help reason and personally go through unfamiliar code paths, but all changes are reviewed, understood, and implemented by me. If not, then I wouldn't personally take my time out and read through the comments and iterate over them. Happy to clarify or adjust anything if there are concerns. |
| CURL_POLL_REMOVE, the application must stop monitoring that socket on | ||
| libcurl's behalf. libcurl does not track idle connections. The pointer | ||
| previously assigned to the socket with curl_multi_assign(3) is forgotten by | ||
| libcurl. |
There was a problem hiding this comment.
This note seems to be better placed in the curl_multi_assign(3) man page!
|
|
||
| When using the multi interface, the close socket callback is invoked when | ||
| libcurl closes a socket it owns. The callback and CURLOPT_CLOSESOCKETDATA(3) | ||
| are copied from the *first* easy handle that creates the connection; |
There was a problem hiding this comment.
| are copied from the *first* easy handle that creates the connection; | |
| are copied from the *first* easy handle that creates the socket used for a connection; |
| are copied from the *first* easy handle that creates the connection; | ||
| changing this option on a subsequent easy handle that reuses the same | ||
| connection has no effect for that connection. The callback is stored with | ||
| the connection because the connection may outlive the easy handle that |
There was a problem hiding this comment.
| the connection because the connection may outlive the easy handle that | |
| the connection because the connection and its associated socket may outlive the easy handle that |
| changing this option on a subsequent easy handle that reuses the same | ||
| connection has no effect for that connection. The callback is stored with | ||
| the connection because the connection may outlive the easy handle that | ||
| created it, so that libcurl can still invoke it when the connection is |
There was a problem hiding this comment.
| created it, so that libcurl can still invoke it when the connection is | |
| created it, so that libcurl can still invoke it when the socket is |
| inherited by a new connection and that connection may live longer | ||
| than the transfer itself in the multi/share handle's connection cache. | ||
|
|
||
| # NOTES ON IDLE CONNECTIONS |
There was a problem hiding this comment.
This note seems to be about reused connections, not idle ones.
| # NOTES ON IDLE CONNECTIONS | |
| # NOTES ON CONNECTION REUSE |
Document observed behavior around idle connections and socket callbacks: - When CURLOPT_CLOSESOCKETFUNCTION is copied and when it is invoked - How idle connections interact with CURLMOPT_SOCKETFUNCTION - Why read/error events may still occur on sockets removed from polling No functional changes.
Co-authored-by: Daniel Stenberg <daniel@haxx.se>
Co-authored-by: Daniel Stenberg <daniel@haxx.se>
Remove socket function and polling details from the close socket callback man page and clarify only the documented lifecycle guarantees.
9713813 to
9c58ed9
Compare
|
Thanks for the update and feedback and I updated the docs with the suggested changes. I renamed the close socket note to connection reuse and adjusted the wording around the first easy handle/socket lifetime. also, I've rebased on current |
|
https://github.com/rakshasa/libtorrent/blob/master/src/net/curl_socket.cc#L377-L450 The above code was necessary to reliably detect libcurl sockets that circumvent the |
|
thanks i've taken a look and updated based on both of the above comments. I removed the extra CURL_POLL_REMOVE sentence from the socket callback page since it was just restating the existing contract. Also, I've reworded the close socket callback note, so that the generic behavior is not described as multi-only. |
|
Thanks! |
This PR documents observed behavior around idle connections and socket callbacks that is not currently described in the documentation.
Specifically:
This behavior is relevant for applications managing large numbers of sockets and custom polling loops (e.g. kqueue/epoll-based integrations).
No functional changes are introduced.
Addresses #20377