Fix tcp::resolver data race in the asio backend and be defensive against empty results #1339
Conversation
…pe is not safe to touch from multiple threads.
BillyONeal
changed the title
Authored-by: Jinming Hu jinming.hu@microsoft.com
| @@ -1050,6 +1050,10 @@ class asio_context final : public request_context, public std::enable_shared_fro | |||
| { | |||
| report_error("Error resolving address", ec, httpclient_errorcode_context::connect); | |||
| } | |||
| else if (endpoints == tcp::resolver::iterator()) | |||
There was a problem hiding this comment.
yuck... raw iterator resolver API!
You are using a deprecated API I believe. If you can require a recent Asio, then you can work with results as a container and just check results.size() == 0. See:
https://www.boost.org/doc/libs/1_72_0/doc/html/boost_asio/reference/ip__basic_resolver_results.html
I would define BOOST_ASIO_NO_DEPRECATED and make sure you aren't using any deprecated APIs. You also don't need the query, use this overload of async_resolve:
Note how there's individual host and service parameters instead of that annoying query type. And note that the completion handler receives a range instead of an iterator.
There was a problem hiding this comment.
If you can require a recent Asio
This has to work back at least as far as 1.58, since that's what Ubuntu 16.04 comes shipped with. Maybe even earlier depending on what RHEL and friends are bundling.
I would define BOOST_ASIO_NO_DEPRECATED and make sure you aren't using any deprecated APIs.
That's #1323 :)
This change is to unblock a specific high profile customer so I don't want to make any risky-for-dependencies changes at the same time. (And this isn't really a can of worms I'm going to open unless forced given cpprestsdk's current "effectively unfunded except for support of the Azure Storage SDK" status)
Move TCP resolver to asio_context rather than asio_client, as that type is not safe to touch from multiple threads. @vinniefalco confirmed over Twitter that constructing resolvers is cheap https://twitter.com/FalcoVinnie/status/1230780333613670401