-
Notifications
You must be signed in to change notification settings - Fork 301
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
HPCC-29992 SOAPCALL check timelimit exceeded after call to getaddrinfo #17631
Conversation
https://track.hpccsystems.com/browse/HPCC-29992 |
Url &connUrl = master->proxyUrlArray.empty() ? url : master->proxyUrlArray.item(0); | ||
ep.set(connUrl.host.get(), connUrl.port); | ||
checkTimeLimitExceeded(&remainingMS); // after ep.set which might make a potentially long getaddrinfo lookup ... |
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.
is it not reasonable to check at the start of the loop (i.e. as it was) as well?
i.e. that may be after it is stuck connecting on a socket for a while and throws an exception, gets another non black listed socket. As it stands, master->timeLimitExceeded will not yet be set in those circumstances afaics, so the next check is the one at the start of the loop.
Also, the connect call is using master->timeoutMS, but probably should be using remainingMS ?
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.
ok, I can add a checkTimeLimitExceeded() at its orig location so its checked there and also after the getaddrinfo() from the ep.set().
The connect() using master->timeoutMS change to use remainingMS is debatable because previously it would have done a single connect with the timeout set to number of attempts * timeout - so to be the same but with a new connect each time we use the timeout for each.
Similarly, there is some discussion if we should checkTimeLimitExceeded() in the connect() retry loop so that multiple connect() attempts do not exceed a timeLimit if provided. Or should timeOut and timeLimit be completely separate ?
So I think this is a good suggestion and perhaps worth of its own JIRA to discuss, but think it should remain as it is here.
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.
I agree with both, keep original checkTimeLimitExceeded(&remainingMS); as well as this new line.
And open a new JIRA to investigate applying time limit to connect (in addition to timeout).
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.
I've added back a checkTimeLimitExceeded() at its original location in commit 2.
@jakesmith @afishbeck if ok with this I will squash.
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.
looks good.
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.
agreed, can you link the new JIRA to this JIRA? - to make it a little easier to find. Thanks.
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.
@mckellyln - a couple of questions.
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.
@mckellyln looks good, please squash.
Signed-off-by: Mark Kelly <[email protected]>
squashed. |
ok to merge |
Type of change:
Checklist:
Smoketest:
Testing:
This together with https://track.hpccsystems.com/browse/HPCC-29991 (#17629) will ensure timelimit is checked after single call to getaddrinfo (from ep.set), before connect().