Fix #3167: avoid infinite wait on failure to init curl handle acquire #3200
Conversation
SergeyRyabinin
force-pushed
the
sr/i3167
branch
from
91bded2 to
390a757
Compare
| const bool isNotEmpty = m_semaphore.wait_for(locker, std::chrono::milliseconds(timeoutMs), | ||
| [&]() { return m_shutdown.load() || !m_resources.empty(); }); | ||
|
|
||
| if (m_shutdown || !isNotEmpty) { |
There was a problem hiding this comment.
nit ! and isNot is kind of confusing could we just use isEmpty instead?
There was a problem hiding this comment.
wait_for returns an enum.
https://en.cppreference.com/w/cpp/thread/condition_variable/wait_for
Better to use this .
enum class cv_status {
no_timeout, => 0
timeout => 1
};
doesm't that mean return on !isNotEmpty is actually returning on no timeout. Is that the intention?
There was a problem hiding this comment.
Also, isNotEmpty can be renamed to isTimeOut for clarity
There was a problem hiding this comment.
Can we add a test case to test the timeout scenario, which should be easy to replicate
There was a problem hiding this comment.
No it doesn't return an enum. Check the spec you linked for the (2) with predicate.
There was a problem hiding this comment.
that's right. looks ok then. if you can add a test case that will be great
There was a problem hiding this comment.
It is a really hard to test this piece of code: curl wrapper is hard-coded to a real c curl APIs, handle container is private and cannot be replaced.
I have a 1y old draft of curl wrapper refactoring: https://github.com/aws/aws-sdk-cpp/tree/sr/curlMulti2 which I still want to have merged one day. I can add curl wrapper unit tests there.
There was a problem hiding this comment.
Added a test on ResourceManager template class.
| bool errorLogged = false; // avoid log explosion on legacy app behavior | ||
| while (!handle && m_poolSize) { | ||
| constexpr unsigned long DEFAULT_TIMEOUT = 1000l; | ||
| handle = m_handleContainer.TryAcquire(std::max(m_connectTimeout, DEFAULT_TIMEOUT)); |
There was a problem hiding this comment.
i think we should use something else besides m_connectTimeout as that maps one to one with CURLOPT_CONNECTTIMEOUT_MS, and we are sort of tying those together. this has its own unique meaning of "aquisition from the pool time", not "DNS lookup time". maybe create a static const/maybe add a config var if this would touch several clients.
There was a problem hiding this comment.
Removed usage of m_connectTimeout, and just set 1s of wait time.
SergeyRyabinin
force-pushed
the
sr/i3167
branch
2 times, most recently
from
06a6b0c to
cd36e12
Compare
SergeyRyabinin
force-pushed
the
sr/i3167
branch
from
cd36e12 to
d15ffac
Compare
SergeyRyabinin
force-pushed
the
sr/i3167
branch
3 times, most recently
from
783ebe7 to
c57b664
Compare
SergeyRyabinin
force-pushed
the
sr/i3167
branch
from
c57b664 to
e1d73f7
Compare
Issue #, if available:
#3167
Description of changes:
Check all that applies:
Check which platforms you have built SDK on to verify the correctness of this PR.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.