multi: fix incorrect multi_easy logic #12665
Conversation
|
@jay why not merge this? |
jay
force-pushed
the
multi_easy
branch
2 times, most recently
from
d5b4dff
to
d506808
Compare
I was unsure about Curl_getconnectinfo but I think I figured it out. Please review the squashme commit. |
lib/connect.c
Outdated
| &data->multi->conn_cache, &find, conn_is_conn); | ||
| data->multi? | ||
| &data->multi->conn_cache: | ||
| &data->multi_easy->conn_cache, &find, conn_is_conn); |
There was a problem hiding this comment.
Is there actually a time when &data->multi->conn_cache is the incorrect choice here?
There was a problem hiding this comment.
I mean, it can hardly reach this code with data->multi being NULL, can it ?
lib/multi.c
Outdated
| @@ -898,7 +898,7 @@ CURLMcode curl_multi_remove_handle(struct Curl_multi *multi, | |||
| /* Remove the association between the connection and the handle */ | |||
| Curl_detach_connection(data); | |||
|
|
|||
| if(data->set.connect_only && !data->multi_easy) { | |||
| if(data->set.connect_only && (data->multi != data->multi_easy)) { | |||
There was a problem hiding this comment.
Why this change? If the easy interface is not used, the multi_easy pointer is NULL so isn't both the old and the new code achieving the same thing?
There was a problem hiding this comment.
If the easy interface is not used, the
multi_easypointer is NULL
sometimes. the rub is if an easy handle is used with the easy interface and then the multi interface. in that case they point to different multis. this pr addresses that issue by prioritizing multi over multi_easy because the former is always the in-use multi. my hang up with Curl_getconnectinfo (which is used in this logic) is, following this scenario, what happens when the easy handle is removed by the user via curl_multi_remove_handle, because that would seem to cause disconnection of connections in multi_easy and not multi. (edit: no, it disconnects the multi connections without affecting the multi_easy connections) afaict, this change fixes a bug by checking that they are different and Curl_getconnectinfo prioritizes multi over multi_easy
|
Now merged. With this, an easy handle can only ever be associated with a single multi handle. |
|
@jay do you want me to continue this? I believe |
- Use data->multi and not data->multi_easy to refer to the active multi. The easy handle's active multi is always data->multi. This is a follow up to 757dfdf which changed curl so that an easy handle used with the easy interface and then multi interface cannot have two different multi handles associated with it at the same time (data->multi_easy from the easy interface and data->multi from the multi interface). Closes #xxxxx
I dropped the fixup commit and changed the other commit's message, if more needs to be done have at it! |
|
@bagder can you have another look, I don't think there's anything left to add |
Prior to this change there were a few places that incorrectly treated data->multi_easy, if != NULL, as the easy handle's active multi.
Background:
The easy handle's active multi is always data->multi. It points to either the user-created multi (if the multi interface is used) or the internally created multi (if the easy interface is used). The internally created multi is always data->multi_easy and is retained until the easy handle is cleaned up.
If an easy handle is used with the easy interface and then the multi interface, data->multi_easy and data->multi will both exist and be different.
Closes #xxxxx
There's one other that I didn't change because I'm not sure if the multi_easy should be checked before multi:
curl/lib/connect.c
Lines 270 to 287 in 8edcfed
Some logic is used to get what should be the connection cache used by the easy handle. I notice
data->state.conn_cacheis not checked so I assume it's not available at that point.