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.

Sign up for GitHub

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

Jump to bottom

HPCC-29992 SOAPCALL check timelimit exceeded after call to getaddrinfo #17631

Merged
merged 1 commit into from
Aug 9, 2023
Merged

HPCC-29992 SOAPCALL check timelimit exceeded after call to getaddrinfo #17631

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions common/thorhelper/thorsoapcall.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2234,6 +2234,7 @@ class CWSCAsyncFor : implements IWSCAsyncFor, public CInterface, public CAsyncFo
checkTimeLimitExceeded(&remainingMS);
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 ...
Copy link
Member

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 ?

Copy link
Contributor Author

@mckellyln mckellyln Aug 3, 2023 •
edited
Loading

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.

Copy link
Member

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

Copy link
Contributor Author

@mckellyln mckellyln Aug 4, 2023 •
edited
Loading

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good.

Copy link
Member

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.

if (strieq(url.method, "https"))
proto = PersistentProtocol::ProtoTLS;
bool shouldClose = false;
Expand Down
Loading