asyn-thread: create a socketpair to wait on for the threaded resolve #4157
Conversation
|
The coveralls warning is just wrong and can be ignored. |
|
What is the next step ?. |
| /* DNS has been resolved, signal client task */ | ||
| if(write(tsd->sock_pair[1], buf, sizeof(buf)) < 0) { | ||
| /* update sock_erro to errno */ | ||
| tsd->sock_error = SOCKERRNO; |
There was a problem hiding this comment.
If this happens, we're in deep trouble since now the main thread won't know the thread is done. Can do anything more than this?
There was a problem hiding this comment.
Main thread is polling for td->tsd.done flag and since we are setting this flag even in case of write error and would result into DNS resolve failure.
CURLcode Curl_resolver_is_resolved(struct connectdata *conn,
struct Curl_dns_entry **entry) {
Curl_mutex_acquire(td->tsd.mtx);
done = td->tsd.done;
Curl_mutex_release(td->tsd.mtx);
if(done) {
getaddrinfo_complete(conn);
if(!conn->async.dns) {
CURLcode result = resolver_error(conn);
destroy_async_data(&conn->async);
return result;
}
destroy_async_data(&conn->async);
*entry = conn->async.dns;
}
Another option can be not to set below mentioned error status, client will not be notified and let connection request gets delay due to application polling ?. With this approach at-least request would get processed.
tsd->sock_error = SOCKERRNO;
Please provide your opinion.
lib/multi.c
Outdated
| #ifdef HAVE_SOCKETPAIR | ||
| /* return CURLM_CALL_MULTI_PERFORM so that client can | ||
| query read socket fd for polling */ | ||
| rc = CURLM_CALL_MULTI_PERFORM; |
There was a problem hiding this comment.
The comment is incorrect, as this return value won't be handled by the "client", it will just cause an internal loop.
There was a problem hiding this comment.
I had added this to let application call curl_multi_perform() based on return status to see if DNS is resolved but as you said this is not required. I have tested with TCP capture and confirmed connection request is sent with in 10 msec after DNS resolution. Hence, removed this change.
There was a problem hiding this comment.
libcurl never returns that anyway, it is only used for an internal loop
There was a problem hiding this comment.
You are right, libcurl doesn't return this status.
It seems earlier version of libcurl was returning this status as my application is still handling CURLM_CALL_MULTI_PERFORM status and needs to be correct.
Thanks for the clarification !
lib/multi.c
Outdated
| /* We're now waiting for an asynchronous name lookup */ | ||
| multistate(data, CURLM_STATE_WAITRESOLVE); | ||
| } |
There was a problem hiding this comment.
this looks like just (unrelated) white space changes, avoid those!
Incorporated review comments. Reverted changes in multi.c
|
CI build has again failed but doesn't seems to be because of my changes. |
|
The travis Apple jobs are struggling right now for some reason and not because of problems in the code. |
|
Thanks a lot! I did some minor edits before I pushed. Are you sure you wouldn't rather have this commit listed in git using your full name? |
|
Thanks a lot Daniel for your continuous support ! I checked the changes you have pushed. I could see you have removed the loop but loop_idx (unused variable is still there.) Regarding below, didn't get this comment completely. I could see it is still listed on my name. |
Oops, sorry for that. I removed the loop since it really doesn't need to clear the unused entries in that array. I'll fix my mistake.
Yes it is. The commit is stored in git as ... and that is fine if you want to keep it like that. I just thought that "amkatyal" might be a user name and not your full name so you perhaps you would rather have the git repository give you credit to your "full name" perhaps as Amit Katyal - but I'm just guessing and asking. |
Understood ! Would prefer to be Amit Katyal. |
I've updated .mailmap accordingly so yes, it should show Amit Katyal now! |
Thank you ! |
No description provided.