From 6bce498bbcc6af4dcbe9fe531f9139564ac22722 Mon Sep 17 00:00:00 2001 From: Marc Horowitz Date: Tue, 28 Feb 2017 14:14:51 -0800 Subject: [PATCH] Tweak CxxMessageQueue to work with unique_ptr Differential Revision: D4560164 fbshipit-source-id: 8975157f5a14d5849365a9e35922a02ac6dc185b --- ReactCommon/cxxreact/BUCK | 2 +- ReactCommon/cxxreact/CxxMessageQueue.cpp | 13 +++-- ReactCommon/cxxreact/CxxMessageQueue.h | 5 +- ReactCommon/cxxreact/tests/BUCK | 1 + .../cxxreact/tests/CxxMessageQueueTest.cpp | 50 ++++++++++++++++--- ReactCommon/cxxreact/tests/jsbigstring.cpp | 2 +- 6 files changed, 61 insertions(+), 12 deletions(-) diff --git a/ReactCommon/cxxreact/BUCK b/ReactCommon/cxxreact/BUCK index 6ebaf1b7685..da1ae79918b 100644 --- a/ReactCommon/cxxreact/BUCK +++ b/ReactCommon/cxxreact/BUCK @@ -56,6 +56,7 @@ if THIS_IS_FBANDROID: # `initOnJSVMThread` to be called before the platform-specific hooks # have been properly initialised. Bad Times(TM). # -- @ashokmenon (2017/01/03) + '//java/com/facebook/java2js:jni', react_native_target('jni/xreact/jni:jni'), react_native_xplat_target('cxxreact/...'), ], @@ -158,7 +159,6 @@ react_library( compiler_flags = [ "-Wall", "-fexceptions", - "-fvisibility=hidden", "-frtti", "-std=c++1y", ] + REACT_LIBRARY_EXTRA_COMPILER_FLAGS, diff --git a/ReactCommon/cxxreact/CxxMessageQueue.cpp b/ReactCommon/cxxreact/CxxMessageQueue.cpp index 090527775eb..e83a3c90a42 100644 --- a/ReactCommon/cxxreact/CxxMessageQueue.cpp +++ b/ReactCommon/cxxreact/CxxMessageQueue.cpp @@ -192,6 +192,7 @@ class CxxMessageQueue::QueueRunner { } void bindToThisThread() { + // TODO: handle nested runloops (either allow them or throw an exception). if (tid_ != std::thread::id{}) { throw std::runtime_error("Message queue already bound to thread."); } @@ -290,9 +291,16 @@ MQRegistry& getMQRegistry() { } } -std::weak_ptr CxxMessageQueue::current() { +std::shared_ptr CxxMessageQueue::current() { auto tid = std::this_thread::get_id(); - return getMQRegistry().find(tid); + return getMQRegistry().find(tid).lock(); +} + +std::function CxxMessageQueue::getUnregisteredRunLoop() { + return [capture=qr_] { + capture->bindToThisThread(); + capture->run(); + }; } std::function CxxMessageQueue::getRunLoop(std::shared_ptr mq) { @@ -300,7 +308,6 @@ std::function CxxMessageQueue::getRunLoop(std::shared_ptrbindToThisThread(); auto tid = std::this_thread::get_id(); - // TODO: handle nested runloops (either allow them or throw an exception). getMQRegistry().registerQueue(tid, weakMq); capture->run(); getMQRegistry().unregister(tid); diff --git a/ReactCommon/cxxreact/CxxMessageQueue.h b/ReactCommon/cxxreact/CxxMessageQueue.h index 4d44bbe7c90..626d058993a 100644 --- a/ReactCommon/cxxreact/CxxMessageQueue.h +++ b/ReactCommon/cxxreact/CxxMessageQueue.h @@ -62,6 +62,9 @@ class CxxMessageQueue : public MessageQueueThread { bool isOnQueue(); + // If this getRunLoop is used, current() will not work. + std::function getUnregisteredRunLoop(); + // This returns a function that will actually run the runloop. // This runloop will return some time after quitSynchronous (or after this is destroyed). // @@ -71,7 +74,7 @@ class CxxMessageQueue : public MessageQueueThread { // Only one thread should run the runloop. static std::function getRunLoop(std::shared_ptr mq); - static std::weak_ptr current(); + static std::shared_ptr current(); private: class QueueRunner; std::shared_ptr qr_; diff --git a/ReactCommon/cxxreact/tests/BUCK b/ReactCommon/cxxreact/tests/BUCK index 8d2a3edab40..f181d7474a9 100644 --- a/ReactCommon/cxxreact/tests/BUCK +++ b/ReactCommon/cxxreact/tests/BUCK @@ -20,6 +20,7 @@ if THIS_IS_FBANDROID: srcs = TEST_SRCS, compiler_flags = [ '-fexceptions', + '-std=c++1y', ], deps = [ '//native/third-party/android-ndk:android', diff --git a/ReactCommon/cxxreact/tests/CxxMessageQueueTest.cpp b/ReactCommon/cxxreact/tests/CxxMessageQueueTest.cpp index 2559432c2e6..f0949cb4043 100644 --- a/ReactCommon/cxxreact/tests/CxxMessageQueueTest.cpp +++ b/ReactCommon/cxxreact/tests/CxxMessageQueueTest.cpp @@ -1,3 +1,5 @@ +// Copyright 2004-present Facebook. All Rights Reserved. + #include #include @@ -29,6 +31,17 @@ std::shared_ptr createAndStartQueue(EventFlag& finishedFlag) { return q; } +std::unique_ptr createAndStartUnregisteredQueue( + EventFlag& finishedFlag) { + auto q = std::make_unique(); + std::thread t([loop=q->getUnregisteredRunLoop(), &finishedFlag] { + loop(); + finishedFlag.set(); + }); + t.detach(); + return q; +} + // This is just used to start up a queue for a test and make sure that it is // actually shut down after the test. struct QueueWithThread { @@ -39,9 +52,7 @@ struct QueueWithThread { ~QueueWithThread() { queue->quitSynchronous(); queue.reset(); - if (!done.wait_until(now() + milliseconds(300))) { - ADD_FAILURE() << "Queue did not exit"; - } + EXPECT_TRUE(done.wait_until(now() + milliseconds(300))) << "Queue did not exit"; } EventFlag done; @@ -53,9 +64,8 @@ TEST(CxxMessageQueue, TestQuit) { EventFlag done; auto q = createAndStartQueue(done); q->quitSynchronous(); - if (!done.wait_until(now() + milliseconds(300))) { - FAIL() << "Queue did not exit runloop after quitSynchronous"; - } + EXPECT_TRUE(done.wait_until(now() + milliseconds(300))) + << "Queue did not exit runloop after quitSynchronous"; } TEST(CxxMessageQueue, TestPostTask) { @@ -69,6 +79,34 @@ TEST(CxxMessageQueue, TestPostTask) { flag.wait(); } +TEST(CxxMessageQueue, TestPostUnregistered) { + EventFlag qdone; + auto q = createAndStartUnregisteredQueue(qdone); + + EventFlag tflag; + q->runOnQueue([&] { + tflag.set(); + }); + tflag.wait(); + + q->quitSynchronous(); + q.reset(); + EXPECT_TRUE(qdone.wait_until(now() + milliseconds(300))) << "Queue did not exit"; +} + +TEST(CxxMessageQueue, TestPostCurrent) { + QueueWithThread qt; + auto q = qt.queue; + + EventFlag flag; + q->runOnQueue([&] { + CxxMessageQueue::current()->runOnQueue([&] { + flag.set(); + }); + }); + flag.wait(); +} + TEST(CxxMessageQueue, TestPostTaskMultiple) { QueueWithThread qt; auto q = qt.queue; diff --git a/ReactCommon/cxxreact/tests/jsbigstring.cpp b/ReactCommon/cxxreact/tests/jsbigstring.cpp index eaa91a9467b..c11b231a349 100644 --- a/ReactCommon/cxxreact/tests/jsbigstring.cpp +++ b/ReactCommon/cxxreact/tests/jsbigstring.cpp @@ -14,7 +14,7 @@ namespace { int tempFileFromString(std::string contents) { std::string tmp {getenv("TMPDIR")}; - tmp += "/temp.XXXXX"; + tmp += "/temp.XXXXXX"; std::vector tmpBuf {tmp.begin(), tmp.end()}; tmpBuf.push_back('\0');