From 6da3b849193da48ae68bad4bab8dda171e0f1aaf Mon Sep 17 00:00:00 2001 From: Chad Parry Date: Fri, 13 May 2016 11:56:03 -0700 Subject: [PATCH] Construct all HHWheelTimer instances with the factory method MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Summary: This diff standardizes `HHWheelTimer` construction. It's not that safe to allow anyone to call `new HHWheelTimer` because the caller needs to remember to call `DelayedDestruction::destroy`. Once callers are made to be safer, I'll be able to change the `HHWheelTimer` to use standard smart pointers instead of `DelayedDestruction`. This picks up some work that I started in D2627941 but had to postpone. This was mostly a mechanical change: ( fbgs -ls 'new HHWheelTimer' ; fbgs -ls 'new folly::HHWheelTimer' ) | xargs perl -pi -e 's/\bnew\s+((?:folly::)?HHWheelTimer)\b/$1::newTimer/g' Then I manually inspected the code. I reverted `folly/io/async/HHWheelTimer.h`, since the `newTimer` factory method is the one place that we still want to call `new HHWheelTimer`. I manually edited `proxygen/lib/utils/SharedWheelTimer.cpp`, since that was using a `shared_ptr` with a custom destructor, which isn't needed anymore. I reverted `common/io/async/d` since the D shim is meant to pass around only raw pointers. I had to replace each instance of `foo.reset(…)` with `foo = …` . Then I made manual edits to `common/clientpool/ClientPool2-inl.h` because that code wants to store the timer in a `ThreadLocalPtr`. Reviewed By: yfeldblum Differential Revision: D3237530 fbshipit-source-id: 96bfb6987098618ad5430c21b1314f385f04a93d --- folly/futures/ThreadWheelTimekeeper.cpp | 9 ++++----- folly/io/async/test/HHWheelTimerTest.cpp | 6 +++--- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/folly/futures/ThreadWheelTimekeeper.cpp b/folly/futures/ThreadWheelTimekeeper.cpp index 0b091e6d..332e4549 100644 --- a/folly/futures/ThreadWheelTimekeeper.cpp +++ b/folly/futures/ThreadWheelTimekeeper.cpp @@ -84,11 +84,10 @@ struct WTCallback : public std::enable_shared_from_this, } // namespace - -ThreadWheelTimekeeper::ThreadWheelTimekeeper() : - thread_([this]{ eventBase_.loopForever(); }), - wheelTimer_(new HHWheelTimer(&eventBase_, std::chrono::milliseconds(1))) -{ +ThreadWheelTimekeeper::ThreadWheelTimekeeper() + : thread_([this] { eventBase_.loopForever(); }), + wheelTimer_( + HHWheelTimer::newTimer(&eventBase_, std::chrono::milliseconds(1))) { eventBase_.waitUntilRunning(); eventBase_.runInEventBaseThread([this]{ // 15 characters max diff --git a/folly/io/async/test/HHWheelTimerTest.cpp b/folly/io/async/test/HHWheelTimerTest.cpp index c8f5a405..df38277d 100644 --- a/folly/io/async/test/HHWheelTimerTest.cpp +++ b/folly/io/async/test/HHWheelTimerTest.cpp @@ -216,9 +216,8 @@ TEST_F(HHWheelTimerTest, CancelTimeout) { */ TEST_F(HHWheelTimerTest, DestroyTimeoutSet) { - HHWheelTimer::UniquePtr t( - new HHWheelTimer(&eventBase, milliseconds(1))); + HHWheelTimer::newTimer(&eventBase, milliseconds(1))); TestTimeout t5_1(t.get(), milliseconds(5)); TestTimeout t5_2(t.get(), milliseconds(5)); @@ -455,7 +454,8 @@ TEST_F(HHWheelTimerTest, cancelAll) { } TEST_F(HHWheelTimerTest, SharedPtr) { - HHWheelTimer::UniquePtr t(new HHWheelTimer(&eventBase, milliseconds(1))); + HHWheelTimer::UniquePtr t( + HHWheelTimer::newTimer(&eventBase, milliseconds(1))); TestTimeout t1; TestTimeout t2; -- 2.34.1