Issue 29408721: Issue 5002 - Fix potential concurrency issues in WebRequest tests. (Closed)
DescriptionIssue 5002 - Fix potential concurrency issues in WebRequest tests.
Patch Set 1 #
Total comments: 8
Patch Set 2 : Fixes from review. #
Total comments: 4
Patch Set 3 : Avoid casting the webRequest pointer. #
Total comments: 4
Patch Set 4 : Fixed the last nits. #MessagesTotal messages: 8
https://codereview.adblockplus.org/29408721/diff/29408722/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29408721/diff/29408722/test/WebRequest.cpp... test/WebRequest.cpp:33: std::set<std::string> r; what about renaming here? r -> headerNames https://codereview.adblockplus.org/29408721/diff/29408722/test/WebRequest.cpp... test/WebRequest.cpp:40: // XXX we currently ignore the result. We should check it actually gets inserted. It would be better to remove XXX. https://codereview.adblockplus.org/29408721/diff/29408722/test/WebRequest.cpp... test/WebRequest.cpp:60: mutable std::map<std::string, std::set<std::string>> lastRequestHeaders; because now it's not only for last requests lastRequestHeaders -> requestsHeaderNames and also for mutex. https://codereview.adblockplus.org/29408721/diff/29408722/test/WebRequest.cpp... test/WebRequest.cpp:254: std::lock_guard<std::mutex> lock(webRequest->lastRequestHeadersMutex); any work with the mutex of webRequest should be moved into a method of webRequest. For instance to a method returning a set of header names and atomically removing it from the map. I would recommend to have a smart pointer to MockWebRequest as a member of the fixture class.
updated patch https://codereview.adblockplus.org/29408721/diff/29408722/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29408721/diff/29408722/test/WebRequest.cpp... test/WebRequest.cpp:33: std::set<std::string> r; On 2017/04/19 10:38:31, sergei wrote: > what about renaming here? > > r -> headerNames Done. https://codereview.adblockplus.org/29408721/diff/29408722/test/WebRequest.cpp... test/WebRequest.cpp:40: // XXX we currently ignore the result. We should check it actually gets inserted. On 2017/04/19 10:38:31, sergei wrote: > It would be better to remove XXX. Acknowledged. https://codereview.adblockplus.org/29408721/diff/29408722/test/WebRequest.cpp... test/WebRequest.cpp:60: mutable std::map<std::string, std::set<std::string>> lastRequestHeaders; On 2017/04/19 10:38:31, sergei wrote: > because now it's not only for last requests > > lastRequestHeaders -> requestsHeaderNames > > and also for mutex. Done. https://codereview.adblockplus.org/29408721/diff/29408722/test/WebRequest.cpp... test/WebRequest.cpp:254: std::lock_guard<std::mutex> lock(webRequest->lastRequestHeadersMutex); On 2017/04/19 10:38:31, sergei wrote: > any work with the mutex of webRequest should be moved into a method of > webRequest. For instance to a method returning a set of header names and > atomically removing it from the map. I would recommend to have a smart pointer > to MockWebRequest as a member of the fixture class. Done.
https://codereview.adblockplus.org/29408721/diff/29417568/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29408721/diff/29417568/test/WebRequest.cpp... test/WebRequest.cpp:64: const auto& iter = requestHeaderNames.find(url); const reference keeps the object from destroying here, but could you please just leave `auto` or `const auto`? https://codereview.adblockplus.org/29408721/diff/29417568/test/WebRequest.cpp... test/WebRequest.cpp:91: AdblockPlus::WebRequestPtr webRequest; What about storing of std::shared_ptr<MockWebRequest> and avoid castings later?
updated patch https://codereview.adblockplus.org/29408721/diff/29417568/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29408721/diff/29417568/test/WebRequest.cpp... test/WebRequest.cpp:64: const auto& iter = requestHeaderNames.find(url); On 2017/04/19 13:17:29, sergei wrote: > const reference keeps the object from destroying here, but could you please just > leave `auto` or `const auto`? indeed, it's better. not sure what I thought here. https://codereview.adblockplus.org/29408721/diff/29417568/test/WebRequest.cpp... test/WebRequest.cpp:91: AdblockPlus::WebRequestPtr webRequest; On 2017/04/19 13:17:29, sergei wrote: > What about storing of std::shared_ptr<MockWebRequest> and avoid castings later? more like std::shared_ptr<T>. Sure I can do that.
LGTM https://codereview.adblockplus.org/29408721/diff/29417570/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29408721/diff/29417570/test/WebRequest.cpp... test/WebRequest.cpp:86: webRequest = std::shared_ptr<T>(new T()); std::make_shared are usually preferable, though here it does not matter. https://codereview.adblockplus.org/29408721/diff/29417570/test/WebRequest.cpp... test/WebRequest.cpp:254: ASSERT_TRUE(webRequest); I'm not sure whether we need it.
Update the patch to make things right. https://codereview.adblockplus.org/29408721/diff/29417570/test/WebRequest.cpp File test/WebRequest.cpp (right): https://codereview.adblockplus.org/29408721/diff/29417570/test/WebRequest.cpp... test/WebRequest.cpp:86: webRequest = std::shared_ptr<T>(new T()); On 2017/04/19 14:44:47, sergei wrote: > std::make_shared are usually preferable, though here it does not matter. let's make it right. https://codereview.adblockplus.org/29408721/diff/29417570/test/WebRequest.cpp... test/WebRequest.cpp:254: ASSERT_TRUE(webRequest); On 2017/04/19 14:44:47, sergei wrote: > I'm not sure whether we need it. I'm probably being paranoid. Even before I'm sure the test was not needed. Removing.
LGTM 
|
