Skip to content
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

asyn-thread: create a socketpair to wait on for the threaded resolve #4157

Closed
wants to merge 1 commit into from

Conversation

@amitkatyal
Copy link

commented Jul 26, 2019

No description provided.

@bagder

This comment has been minimized.

Copy link
Member

commented Jul 27, 2019

The coveralls warning is just wrong and can be ignored.

@amitkatyal

This comment has been minimized.

Copy link
Author

commented Jul 27, 2019

What is the next step ?.

lib/asyn-thread.c Outdated Show resolved Hide resolved
lib/asyn-thread.c Outdated Show resolved Hide resolved
lib/asyn-thread.c Outdated Show resolved Hide resolved
lib/asyn-thread.c Outdated Show resolved Hide resolved
lib/asyn-thread.c Outdated Show resolved Hide resolved
/* 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;

This comment has been minimized.

Copy link
@bagder

bagder Jul 28, 2019

Member

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?

This comment has been minimized.

Copy link
@amitkatyal

amitkatyal Jul 29, 2019

Author

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/asyn-thread.c Outdated Show resolved Hide resolved
lib/asyn-thread.c Outdated Show resolved Hide resolved
lib/asyn-thread.c Outdated Show resolved Hide resolved
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;

This comment has been minimized.

Copy link
@bagder

bagder Jul 28, 2019

Member

The comment is incorrect, as this return value won't be handled by the "client", it will just cause an internal loop.

This comment has been minimized.

Copy link
@amitkatyal

amitkatyal Jul 29, 2019

Author

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.

This comment has been minimized.

Copy link
@bagder

bagder Jul 29, 2019

Member

libcurl never returns that anyway, it is only used for an internal loop

This comment has been minimized.

Copy link
@amitkatyal

amitkatyal Jul 29, 2019

Author

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 !

@amitkatyal amitkatyal force-pushed the amitkatyal:async-dns branch from 20b7713 to 0eef22f Jul 29, 2019

lib/multi.c Outdated
/* We're now waiting for an asynchronous name lookup */
multistate(data, CURLM_STATE_WAITRESOLVE);
}

This comment has been minimized.

Copy link
@bagder

bagder Jul 29, 2019

Member

this looks like just (unrelated) white space changes, avoid those!

This comment has been minimized.

Copy link
@amitkatyal
amkatyal
asyn-thread: create a socketpair to wait on for the threaded resolve
Incorporated review comments.
Reverted changes in multi.c

@amitkatyal amitkatyal force-pushed the amitkatyal:async-dns branch from 0eef22f to 6aa7914 Jul 29, 2019

@amitkatyal

This comment has been minimized.

Copy link
Author

commented Jul 30, 2019

CI build has again failed but doesn't seems to be because of my changes.
Do I need to take any action ?

@bagder

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

The travis Apple jobs are struggling right now for some reason and not because of problems in the code.

@bagder bagder closed this in eb9a604 Jul 30, 2019

@bagder

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

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?

@amitkatyal

This comment has been minimized.

Copy link
Author

commented Jul 30, 2019

Thanks a lot Daniel for your continuous support !
It was really great experience working on open source project.

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.
Are you sure you wouldn't rather have this commit listed in git using your full name?

@bagder

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

I could see you have removed the loop but loop_idx (unused variable is still there.)

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.

I could see it is still listed on my name.

Yes it is. The commit is stored in git as

Author: amkatyal <amkatyal@cisco.com>

... 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.

@amitkatyal

This comment has been minimized.

Copy link
Author

commented Jul 30, 2019

I could see you have removed the loop but loop_idx (unused variable is still there.)

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.

I could see it is still listed on my name.

Yes it is. The commit is stored in git as

Author: amkatyal <amkatyal@cisco.com>

... 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.
If we can change it now, Please let me know. Thanks you.

@bagder

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

If we can change it now, Please let me know. Thanks you.

I've updated .mailmap accordingly so yes, it should show Amit Katyal now!

@amitkatyal

This comment has been minimized.

Copy link
Author

commented Jul 30, 2019

If we can change it now, Please let me know. Thanks you.

I've updated .mailmap accordingly so yes, it should show Amit Katyal now!

Thank you !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.