From b56b51fcafabc678e90474871cd282450949152a Mon Sep 17 00:00:00 2001 From: Justin Berger Date: Fri, 17 Nov 2017 13:43:51 -0700 Subject: utility: Disabled copy ctors in thread classes --- Source/cm_thread.hxx | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Source/cm_thread.hxx b/Source/cm_thread.hxx index b8c25c7..ec5fe1d 100644 --- a/Source/cm_thread.hxx +++ b/Source/cm_thread.hxx @@ -9,6 +9,7 @@ namespace cm { class mutex { + CM_DISABLE_COPY(mutex) uv_mutex_t _M_; public: @@ -24,6 +25,7 @@ template class lock_guard { T& _mutex; + CM_DISABLE_COPY(lock_guard) public: lock_guard(T& m) @@ -37,6 +39,7 @@ public: class shared_mutex { uv_rwlock_t _M_; + CM_DISABLE_COPY(shared_mutex) public: shared_mutex() { uv_rwlock_init(&_M_); } @@ -55,6 +58,7 @@ template class shared_lock { T& _mutex; + CM_DISABLE_COPY(shared_lock) public: shared_lock(T& m) @@ -68,6 +72,8 @@ public: template class unique_lock : public lock_guard { + CM_DISABLE_COPY(unique_lock) + public: unique_lock(T& m) : lock_guard(m) -- cgit v0.12 From 90f8db269fcc6978badd93b5f985dee2e01feab3 Mon Sep 17 00:00:00 2001 From: Justin Berger Date: Thu, 9 Nov 2017 20:52:19 -0700 Subject: tests: unconditionally enabled server tests --- Tests/CMakeLists.txt | 4 +--- Tests/CMakeServerLib/testServerBuffering.cpp | 5 ++--- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/Tests/CMakeLists.txt b/Tests/CMakeLists.txt index 4a7b8c9..18fed75 100644 --- a/Tests/CMakeLists.txt +++ b/Tests/CMakeLists.txt @@ -149,9 +149,7 @@ if(BUILD_TESTING) if(NOT CMake_TEST_EXTERNAL_CMAKE) add_subdirectory(CMakeLib) - if(CMake_TEST_SERVER_MODE) - add_subdirectory(CMakeServerLib) - endif() + add_subdirectory(CMakeServerLib) endif() add_subdirectory(CMakeOnly) add_subdirectory(RunCMake) diff --git a/Tests/CMakeServerLib/testServerBuffering.cpp b/Tests/CMakeServerLib/testServerBuffering.cpp index 97be891..7330ead 100644 --- a/Tests/CMakeServerLib/testServerBuffering.cpp +++ b/Tests/CMakeServerLib/testServerBuffering.cpp @@ -1,7 +1,6 @@ #include "cmConnection.h" #include "cmServerConnection.h" #include -#include #include #include @@ -51,8 +50,8 @@ int testServerBuffering(int, char** const) std::unique_ptr(new cmServerBufferStrategy); std::vector response; std::string rawBuffer; - for (size_t i = 0; i < fullMessage.size(); i++) { - rawBuffer += fullMessage[i]; + for (auto& messageChar : fullMessage) { + rawBuffer += messageChar; std::string packet = bufferingStrategy->BufferMessage(rawBuffer); do { if (!packet.empty() && packet != "\r\n") { -- cgit v0.12 From a3abb85c6ff5aebe2220ae612512a434b491eedd Mon Sep 17 00:00:00 2001 From: Justin Berger Date: Thu, 27 Jul 2017 07:11:07 -0600 Subject: Add RAII handles for libuv handle types The `uv_*_t` handle types are closed by `uv_close`, but the semantics are tricky. Calling `uv_close` may not close immediately. Instead it hands ownership to the uv loop to which the handle is currently attached. When the loop decides to close it, a callback is used to allow the `uv_close` caller to free resources. Provide an abstraction layer as `cm::uv_*_ptr` types corresponding to the `uv_*_t` handle types. Each pointer is either empty (`nullptr`) or has an initialized handle attached to a loop. Use move semantics to ensure a single owner of the handle so that clients can predict when the handle is destroyed. --- Source/CMakeLists.txt | 1 + Source/cmUVHandlePtr.cxx | 198 ++++++++++++++++++++++++++++++++++++++++++++++ Source/cmUVHandlePtr.h | 200 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 399 insertions(+) create mode 100644 Source/cmUVHandlePtr.cxx create mode 100644 Source/cmUVHandlePtr.h diff --git a/Source/CMakeLists.txt b/Source/CMakeLists.txt index 33ab093..88c63e1 100644 --- a/Source/CMakeLists.txt +++ b/Source/CMakeLists.txt @@ -1029,6 +1029,7 @@ list(APPEND _tools cmake) target_link_libraries(cmake CMakeLib) add_library(CMakeServerLib + cmUVHandlePtr.h cmUVHandlePtr.cxx cmConnection.h cmConnection.cxx cmFileMonitor.cxx cmFileMonitor.h cmPipeConnection.cxx cmPipeConnection.h diff --git a/Source/cmUVHandlePtr.cxx b/Source/cmUVHandlePtr.cxx new file mode 100644 index 0000000..214c7b2 --- /dev/null +++ b/Source/cmUVHandlePtr.cxx @@ -0,0 +1,198 @@ +/* Distributed under the OSI-approved BSD 3-Clause License. See accompanying + file Copyright.txt or https://cmake.org/licensing for details. */ +#define cmUVHandlePtr_cxx +#include "cmUVHandlePtr.h" + +#include +#include + +#include "cm_thread.hxx" +#include "cm_uv.h" + +namespace cm { + +static void close_delete(uv_handle_t* h) +{ + free(h); +} + +template +static void default_delete(T* type_handle) +{ + auto handle = reinterpret_cast(type_handle); + if (handle) { + assert(!uv_is_closing(handle)); + if (!uv_is_closing(handle)) { + uv_close(handle, &close_delete); + } + } +} + +/** + * Encapsulates delete logic for a given handle type T + */ +template +struct uv_handle_deleter +{ + void operator()(T* type_handle) const { default_delete(type_handle); } +}; + +template +void uv_handle_ptr_base_::allocate(void* data) +{ + reset(); + + /* + We use calloc since we know all these types are c structs + and we just want to 0 init them. New would do the same thing; + but casting from uv_handle_t to certain other types -- namely + uv_timer_t -- triggers a cast_align warning on certain systems. + */ + handle.reset(static_cast(calloc(1, sizeof(T))), uv_handle_deleter()); + handle->data = data; +} + +template +void uv_handle_ptr_base_::reset() +{ + handle.reset(); +} + +template +uv_handle_ptr_base_::operator uv_handle_t*() +{ + return reinterpret_cast(handle.get()); +} + +template +T* uv_handle_ptr_base_::operator->() const noexcept +{ + return handle.get(); +} + +template +T* uv_handle_ptr_base_::get() const +{ + return handle.get(); +} + +template +uv_handle_ptr_::operator T*() const +{ + return this->handle.get(); +} + +template <> +struct uv_handle_deleter +{ + /*** + * Wile uv_async_send is itself thread-safe, there are + * no strong guarantees that close hasn't already been + * called on the handle; and that it might be deleted + * as the send call goes through. This mutex guards + * against that. + * + * The shared_ptr here is to allow for copy construction + * which is mandated by the standard for Deleter on + * shared_ptrs. + */ + std::shared_ptr handleMutex; + + uv_handle_deleter() + : handleMutex(std::make_shared()) + { + } + + void operator()(uv_async_t* handle) + { + cm::lock_guard lock(*handleMutex); + default_delete(handle); + } +}; + +void uv_async_ptr::send() +{ + auto deleter = std::get_deleter>(this->handle); + assert(deleter); + + cm::lock_guard lock(*deleter->handleMutex); + if (this->handle) { + uv_async_send(*this); + } +} + +int uv_async_ptr::init(uv_loop_t& loop, uv_async_cb async_cb, void* data) +{ + allocate(data); + return uv_async_init(&loop, handle.get(), async_cb); +} + +template <> +struct uv_handle_deleter +{ + void operator()(uv_signal_t* handle) const + { + if (handle) { + uv_signal_stop(handle); + default_delete(handle); + } + } +}; + +int uv_signal_ptr::init(uv_loop_t& loop, void* data) +{ + allocate(data); + return uv_signal_init(&loop, handle.get()); +} + +int uv_signal_ptr::start(uv_signal_cb cb, int signum) +{ + assert(handle); + return uv_signal_start(*this, cb, signum); +} + +void uv_signal_ptr::stop() +{ + if (handle) { + uv_signal_stop(*this); + } +} + +int uv_pipe_ptr::init(uv_loop_t& loop, int ipc, void* data) +{ + allocate(data); + return uv_pipe_init(&loop, *this, ipc); +} + +uv_pipe_ptr::operator uv_stream_t*() const +{ + return reinterpret_cast(handle.get()); +} + +uv_tty_ptr::operator uv_stream_t*() const +{ + return reinterpret_cast(handle.get()); +} + +int uv_tty_ptr::init(uv_loop_t& loop, int fd, int readable, void* data) +{ + allocate(data); + return uv_tty_init(&loop, *this, fd, readable); +} + +template class uv_handle_ptr_base_; + +#define UV_HANDLE_PTR_INSTANTIATE_EXPLICIT(NAME) \ + template class uv_handle_ptr_base_; \ + template class uv_handle_ptr_; + +UV_HANDLE_PTR_INSTANTIATE_EXPLICIT(async) + +UV_HANDLE_PTR_INSTANTIATE_EXPLICIT(signal) + +UV_HANDLE_PTR_INSTANTIATE_EXPLICIT(pipe) + +UV_HANDLE_PTR_INSTANTIATE_EXPLICIT(stream) + +UV_HANDLE_PTR_INSTANTIATE_EXPLICIT(tty) +} diff --git a/Source/cmUVHandlePtr.h b/Source/cmUVHandlePtr.h new file mode 100644 index 0000000..a251c5f --- /dev/null +++ b/Source/cmUVHandlePtr.h @@ -0,0 +1,200 @@ +/* Distributed under the OSI-approved BSD 3-Clause License. See accompanying + file Copyright.txt or https://cmake.org/licensing for details. */ +#pragma once +#include "cmConfigure.h" // IWYU pragma: keep + +#include +#include +#include +#include + +#include "cm_uv.h" + +#define CM_PERFECT_FWD_CTOR(Class, FwdTo) \ + template \ + Class(Args&&... args) \ + : FwdTo(std::forward(args)...) \ + { \ + } + +namespace cm { + +/*** +* RAII class to simplify and insure the safe usage of uv_*_t types. This +* includes making sure resources are properly freed and contains casting +* operators which allow for passing into relevant uv_* functions. +* +*@tparam T actual uv_*_t type represented. +*/ +template +class uv_handle_ptr_base_ +{ +protected: + template + friend class uv_handle_ptr_base_; + + /** + * This must be a pointer type since the handle can outlive this class. + * When uv_close is eventually called on the handle, the memory the + * handle inhabits must be valid until the close callback is called + * which can be later on in the loop. + */ + std::shared_ptr handle; + + /** + * Allocate memory for the type and optionally set it's 'data' pointer. + * Protected since this should only be called for an appropriate 'init' + * call. + * + * @param data data pointer to set + */ + void allocate(void* data = nullptr); + +public: + CM_DISABLE_COPY(uv_handle_ptr_base_) + uv_handle_ptr_base_(uv_handle_ptr_base_&&) noexcept; + uv_handle_ptr_base_& operator=(uv_handle_ptr_base_&&) noexcept; + + /** + * This move constructor allows us to move out of a more specialized + * uv type into a less specialized one. The only constraint is that + * the right hand side is castable to T. + * + * This allows you to return uv_handle_ptr or uv_stream_ptr from a function + * that initializes something like uv_pipe_ptr or uv_tcp_ptr and interact + * and clean up after it without caring about the exact type. + */ + template ::value>::type> + uv_handle_ptr_base_(S&& rhs) + { + // This will force a compiler error if rhs doesn't have a casting + // operator to get T* + this->handle = std::shared_ptr(rhs.handle, rhs); + rhs.handle.reset(); + } + + // Dtor and ctor need to be inline defined like this for default ctors and + // dtors to work. + uv_handle_ptr_base_() {} + uv_handle_ptr_base_(std::nullptr_t) {} + ~uv_handle_ptr_base_() { reset(); } + + /** + * Properly close the handle if needed and sets the inner handle to nullptr + */ + void reset(); + + /** + * Allow less verbose calling of uv_handle_* functions + * @return reinterpreted handle + */ + operator uv_handle_t*(); + + T* get() const; + T* operator->() const noexcept; +}; + +template +inline uv_handle_ptr_base_::uv_handle_ptr_base_( + uv_handle_ptr_base_&&) noexcept = default; +template +inline uv_handle_ptr_base_& uv_handle_ptr_base_::operator=( + uv_handle_ptr_base_&&) noexcept = default; + +/** + * While uv_handle_ptr_base_ only exposes uv_handle_t*, this exposes uv_T_t* + * too. It is broken out like this so we can reuse most of the code for the + * uv_handle_ptr class. + */ +template +class uv_handle_ptr_ : public uv_handle_ptr_base_ +{ + template + friend class uv_handle_ptr_; + +public: + CM_PERFECT_FWD_CTOR(uv_handle_ptr_, uv_handle_ptr_base_); + + /*** + * Allow less verbose calling of uv_ functions + * @return reinterpreted handle + */ + operator T*() const; +}; + +/*** + * This specialization is required to avoid duplicate 'operator uv_handle_t*()' + * declarations + */ +template <> +class uv_handle_ptr_ : public uv_handle_ptr_base_ +{ +public: + CM_PERFECT_FWD_CTOR(uv_handle_ptr_, uv_handle_ptr_base_); +}; + +class uv_async_ptr : public uv_handle_ptr_ +{ +public: + CM_PERFECT_FWD_CTOR(uv_async_ptr, uv_handle_ptr_); + + int init(uv_loop_t& loop, uv_async_cb async_cb, void* data = nullptr); + + void send(); +}; + +struct uv_signal_ptr : public uv_handle_ptr_ +{ + CM_PERFECT_FWD_CTOR(uv_signal_ptr, uv_handle_ptr_); + + int init(uv_loop_t& loop, void* data = nullptr); + + int start(uv_signal_cb cb, int signum); + + void stop(); +}; + +struct uv_pipe_ptr : public uv_handle_ptr_ +{ + CM_PERFECT_FWD_CTOR(uv_pipe_ptr, uv_handle_ptr_); + + operator uv_stream_t*() const; + + int init(uv_loop_t& loop, int ipc, void* data = nullptr); +}; + +struct uv_tty_ptr : public uv_handle_ptr_ +{ + CM_PERFECT_FWD_CTOR(uv_tty_ptr, uv_handle_ptr_); + + operator uv_stream_t*() const; + + int init(uv_loop_t& loop, int fd, int readable, void* data = nullptr); +}; + +typedef uv_handle_ptr_ uv_stream_ptr; +typedef uv_handle_ptr_ uv_handle_ptr; + +#ifndef cmUVHandlePtr_cxx + +extern template class uv_handle_ptr_base_; + +#define UV_HANDLE_PTR_INSTANTIATE_EXTERN(NAME) \ + extern template class uv_handle_ptr_base_; \ + extern template class uv_handle_ptr_; + +UV_HANDLE_PTR_INSTANTIATE_EXTERN(async) + +UV_HANDLE_PTR_INSTANTIATE_EXTERN(signal) + +UV_HANDLE_PTR_INSTANTIATE_EXTERN(pipe) + +UV_HANDLE_PTR_INSTANTIATE_EXTERN(stream) + +UV_HANDLE_PTR_INSTANTIATE_EXTERN(tty) + +#undef UV_HANDLE_PTR_INSTANTIATE_EXTERN + +#endif +} -- cgit v0.12 From f43b9219c738944fea33043b1985696c862c84ac Mon Sep 17 00:00:00 2001 From: Justin Berger Date: Thu, 9 Nov 2017 20:52:32 -0700 Subject: tests: Added tests to verify UV RAII semantics/constructs --- Tests/CMakeServerLib/CMakeLists.txt | 4 + Tests/CMakeServerLib/testUVRAII.cxx | 179 ++++++++++++++++++++++++++++++++++++ 2 files changed, 183 insertions(+) create mode 100644 Tests/CMakeServerLib/testUVRAII.cxx diff --git a/Tests/CMakeServerLib/CMakeLists.txt b/Tests/CMakeServerLib/CMakeLists.txt index f5351fd..f1ca2a4 100644 --- a/Tests/CMakeServerLib/CMakeLists.txt +++ b/Tests/CMakeServerLib/CMakeLists.txt @@ -6,12 +6,16 @@ include_directories( set(CMakeServerLib_TESTS testServerBuffering + testUVRAII ) create_test_sourcelist(CMakeLib_TEST_SRCS CMakeServerLibTests.cxx ${CMakeServerLib_TESTS}) add_executable(CMakeServerLibTests ${CMakeLib_TEST_SRCS}) target_link_libraries(CMakeServerLibTests CMakeLib CMakeServerLib) +SET_PROPERTY(TARGET CMakeServerLibTests PROPERTY C_CLANG_TIDY "") +SET_PROPERTY(TARGET CMakeServerLibTests PROPERTY CXX_CLANG_TIDY "") + foreach(test ${CMakeServerLib_TESTS}) add_test(CMakeServerLib.${test} CMakeServerLibTests ${test} ${${test}_ARGS}) endforeach() diff --git a/Tests/CMakeServerLib/testUVRAII.cxx b/Tests/CMakeServerLib/testUVRAII.cxx new file mode 100644 index 0000000..7ecef39 --- /dev/null +++ b/Tests/CMakeServerLib/testUVRAII.cxx @@ -0,0 +1,179 @@ +#include "cmUVHandlePtr.h" + +#include +#include +#include +#include + +#include "cm_uv.h" + +static void signal_reset_fn(uv_async_t* handle) +{ + auto ptr = static_cast(handle->data); + ptr->reset(); +} + +// A common pattern is to use an async signal to shutdown the server. +static bool testAsyncShutdown() +{ + uv_loop_t Loop; + auto err = uv_loop_init(&Loop); + if (err != 0) { + std::cerr << "Could not init loop" << std::endl; + return false; + } + + { + cm::uv_async_ptr signal; + signal.init(Loop, &signal_reset_fn, &signal); + + std::thread([&] { + std::this_thread::sleep_for(std::chrono::seconds(2)); + signal.send(); + }).detach(); + + if (uv_run(&Loop, UV_RUN_DEFAULT) != 0) { + std::cerr << "Unclean exit state in testAsyncDtor" << std::endl; + return false; + } + + if (signal.get()) { + std::cerr << "Loop exited with signal not being cleaned up" << std::endl; + return false; + } + } + + uv_loop_close(&Loop); + + return true; +} + +static void signal_fn(uv_async_t*) +{ +} + +// Async dtor is sort of a pain; since it locks a mutex we must be sure its +// dtor always calls reset otherwise the mutex is deleted then locked. +static bool testAsyncDtor() +{ + uv_loop_t Loop; + auto err = uv_loop_init(&Loop); + if (err != 0) { + std::cerr << "Could not init loop" << std::endl; + return false; + } + + { + cm::uv_async_ptr signal; + signal.init(Loop, signal_fn); + } + + if (uv_run(&Loop, UV_RUN_DEFAULT) != 0) { + std::cerr << "Unclean exit state in testAsyncDtor" << std::endl; + return false; + } + + uv_loop_close(&Loop); + + return true; +} + +// Async needs a relatively stateful deleter; make sure that is properly +// accounted for and doesn't try to hold on to invalid state when it is +// moved +static bool testAsyncMove() +{ + uv_loop_t Loop; + auto err = uv_loop_init(&Loop); + if (err != 0) { + std::cerr << "Could not init loop" << std::endl; + return false; + } + + { + cm::uv_async_ptr signal; + { + cm::uv_async_ptr signalTmp; + signalTmp.init(Loop, signal_fn); + signal = std::move(signalTmp); + } + } + + if (uv_run(&Loop, UV_RUN_DEFAULT) != 0) { + std::cerr << "Unclean exit state in testAsyncDtor" << std::endl; + return false; + } + + uv_loop_close(&Loop); + return true; +} + +// When a type is castable to another uv type (pipe -> stream) here, +// and the deleter is convertible as well, we should allow moves from +// one type to the other. +static bool testCrossAssignment() +{ + uv_loop_t Loop; + auto err = uv_loop_init(&Loop); + if (err != 0) { + std::cerr << "Could not init loop" << std::endl; + return false; + } + + { + cm::uv_pipe_ptr pipe; + pipe.init(Loop, 0); + + cm::uv_stream_ptr stream = std::move(pipe); + if (pipe.get()) { + std::cerr << "Move should be sure to invalidate the previous ptr" + << std::endl; + return false; + } + cm::uv_handle_ptr handle = std::move(stream); + if (stream.get()) { + std::cerr << "Move should be sure to invalidate the previous ptr" + << std::endl; + return false; + } + } + + if (uv_run(&Loop, UV_RUN_DEFAULT) != 0) { + std::cerr << "Unclean exit state in testCrossAssignment" << std::endl; + return false; + } + + uv_loop_close(&Loop); + return true; +} + +// This test can't fail at run time; but this makes sure we have all our move +// ctors created correctly. +static bool testAllMoves() +{ + using namespace cm; + struct allTypes + { + uv_stream_ptr _7; + uv_tty_ptr _9; + uv_pipe_ptr _12; + uv_async_ptr _13; + uv_signal_ptr _14; + uv_handle_ptr _15; + }; + + allTypes a; + allTypes b(std::move(a)); + allTypes c = std::move(b); + return true; +}; + +int testUVRAII(int, char** const) +{ + if ((testAsyncShutdown() && + testAsyncDtor() & testAsyncMove() & testCrossAssignment() & + testAllMoves()) == 0) { + return -1; + } + return 0; +} -- cgit v0.12 From 1e9b7d3ceb882d3feb59324b6e55d40cc795576b Mon Sep 17 00:00:00 2001 From: Justin Berger Date: Sun, 23 Jul 2017 12:31:13 -0600 Subject: server: Switched to a auto model for handles --- Source/cmConnection.cxx | 19 +++++------- Source/cmConnection.h | 5 +-- Source/cmPipeConnection.cxx | 51 ++++++++++++------------------- Source/cmPipeConnection.h | 4 +-- Source/cmServer.cxx | 67 ++++++++++++++++++++-------------------- Source/cmServer.h | 10 +++--- Source/cmServerConnection.cxx | 71 +++++++++++++------------------------------ Source/cmServerConnection.h | 6 ++-- 8 files changed, 96 insertions(+), 137 deletions(-) diff --git a/Source/cmConnection.cxx b/Source/cmConnection.cxx index 28ba12c..50e1936 100644 --- a/Source/cmConnection.cxx +++ b/Source/cmConnection.cxx @@ -26,7 +26,7 @@ void cmEventBasedConnection::on_alloc_buffer(uv_handle_t* handle, void cmEventBasedConnection::on_read(uv_stream_t* stream, ssize_t nread, const uv_buf_t* buf) { - auto conn = reinterpret_cast(stream->data); + auto conn = static_cast(stream->data); if (conn) { if (nread >= 0) { conn->ReadData(std::string(buf->base, buf->base + nread)); @@ -55,7 +55,7 @@ void cmEventBasedConnection::on_write(uv_write_t* req, int status) void cmEventBasedConnection::on_new_connection(uv_stream_t* stream, int status) { (void)(status); - auto conn = reinterpret_cast(stream->data); + auto conn = static_cast(stream->data); if (conn) { conn->Connect(stream); @@ -76,7 +76,7 @@ void cmEventBasedConnection::WriteData(const std::string& _data) #endif auto data = _data; - assert(this->WriteStream); + assert(this->WriteStream.get()); if (BufferStrategy) { data = BufferStrategy->BufferOutMessage(data); } @@ -87,8 +87,7 @@ void cmEventBasedConnection::WriteData(const std::string& _data) req->req.data = this; req->buf = uv_buf_init(new char[ds], static_cast(ds)); memcpy(req->buf.base, data.c_str(), ds); - uv_write(reinterpret_cast(req), - static_cast(this->WriteStream), &req->buf, 1, + uv_write(reinterpret_cast(req), this->WriteStream, &req->buf, 1, on_write); } @@ -156,13 +155,11 @@ bool cmConnection::OnServeStart(std::string* errString) bool cmEventBasedConnection::OnConnectionShuttingDown() { - if (this->WriteStream) { + if (this->WriteStream.get()) { this->WriteStream->data = nullptr; } - if (this->ReadStream) { - this->ReadStream->data = nullptr; - } - this->ReadStream = nullptr; - this->WriteStream = nullptr; + + WriteStream.reset(); + return true; } diff --git a/Source/cmConnection.h b/Source/cmConnection.h index ddb7744..ce2d2dc 100644 --- a/Source/cmConnection.h +++ b/Source/cmConnection.h @@ -5,6 +5,7 @@ #include "cmConfigure.h" // IWYU pragma: keep +#include "cmUVHandlePtr.h" #include "cm_uv.h" #include @@ -107,8 +108,6 @@ public: bool OnConnectionShuttingDown() override; virtual void OnDisconnect(int errorCode); - uv_stream_t* ReadStream = nullptr; - uv_stream_t* WriteStream = nullptr; static void on_close(uv_handle_t* handle); @@ -119,6 +118,8 @@ public: } protected: + cm::uv_stream_ptr WriteStream; + std::string RawReadBuffer; std::unique_ptr BufferStrategy; diff --git a/Source/cmPipeConnection.cxx b/Source/cmPipeConnection.cxx index 9e565f6..3dab2f0 100644 --- a/Source/cmPipeConnection.cxx +++ b/Source/cmPipeConnection.cxx @@ -2,6 +2,8 @@ file Copyright.txt or https://cmake.org/licensing for details. */ #include "cmPipeConnection.h" +#include + #include "cmServer.h" cmPipeConnection::cmPipeConnection(const std::string& name, @@ -13,39 +15,33 @@ cmPipeConnection::cmPipeConnection(const std::string& name, void cmPipeConnection::Connect(uv_stream_t* server) { - if (this->ClientPipe) { + if (this->WriteStream.get()) { // Accept and close all pipes but the first: - uv_pipe_t* rejectPipe = new uv_pipe_t(); + cm::uv_pipe_ptr rejectPipe; + + rejectPipe.init(*this->Server->GetLoop(), 0); + uv_accept(server, rejectPipe); - uv_pipe_init(this->Server->GetLoop(), rejectPipe, 0); - uv_accept(server, reinterpret_cast(rejectPipe)); - uv_close(reinterpret_cast(rejectPipe), - &on_close_delete); return; } - this->ClientPipe = new uv_pipe_t(); - uv_pipe_init(this->Server->GetLoop(), this->ClientPipe, 0); - this->ClientPipe->data = static_cast(this); - auto client = reinterpret_cast(this->ClientPipe); - if (uv_accept(server, client) != 0) { - uv_close(reinterpret_cast(client), - &on_close_delete); - this->ClientPipe = nullptr; + cm::uv_pipe_ptr ClientPipe; + ClientPipe.init(*this->Server->GetLoop(), 0, + static_cast(this)); + + if (uv_accept(server, ClientPipe) != 0) { return; } - this->ReadStream = client; - this->WriteStream = client; - uv_read_start(this->ReadStream, on_alloc_buffer, on_read); + uv_read_start(ClientPipe, on_alloc_buffer, on_read); + WriteStream = std::move(ClientPipe); Server->OnConnected(this); } bool cmPipeConnection::OnServeStart(std::string* errorMessage) { - this->ServerPipe = new uv_pipe_t(); - uv_pipe_init(this->Server->GetLoop(), this->ServerPipe, 0); - this->ServerPipe->data = static_cast(this); + this->ServerPipe.init(*this->Server->GetLoop(), 0, + static_cast(this)); int r; if ((r = uv_pipe_bind(this->ServerPipe, this->PipeName.c_str())) != 0) { @@ -53,8 +49,8 @@ bool cmPipeConnection::OnServeStart(std::string* errorMessage) ": " + uv_err_name(r); return false; } - auto serverStream = reinterpret_cast(this->ServerPipe); - if ((r = uv_listen(serverStream, 1, on_new_connection)) != 0) { + + if ((r = uv_listen(this->ServerPipe, 1, on_new_connection)) != 0) { *errorMessage = std::string("Internal Error listening on ") + this->PipeName + ": " + uv_err_name(r); return false; @@ -65,18 +61,11 @@ bool cmPipeConnection::OnServeStart(std::string* errorMessage) bool cmPipeConnection::OnConnectionShuttingDown() { - if (this->ClientPipe) { - uv_close(reinterpret_cast(this->ClientPipe), - &on_close_delete); + if (this->WriteStream.get()) { this->WriteStream->data = nullptr; } - uv_close(reinterpret_cast(this->ServerPipe), - &on_close_delete); - this->ClientPipe = nullptr; - this->ServerPipe = nullptr; - this->WriteStream = nullptr; - this->ReadStream = nullptr; + this->ServerPipe.reset(); return cmEventBasedConnection::OnConnectionShuttingDown(); } diff --git a/Source/cmPipeConnection.h b/Source/cmPipeConnection.h index 7b89842..49f9fdf 100644 --- a/Source/cmPipeConnection.h +++ b/Source/cmPipeConnection.h @@ -4,6 +4,7 @@ #include "cmConfigure.h" // IWYU pragma: keep +#include "cmUVHandlePtr.h" #include #include "cmConnection.h" @@ -23,6 +24,5 @@ public: private: const std::string PipeName; - uv_pipe_t* ServerPipe = nullptr; - uv_pipe_t* ClientPipe = nullptr; + cm::uv_pipe_ptr ServerPipe; }; diff --git a/Source/cmServer.cxx b/Source/cmServer.cxx index 9af4c0a..ac42fde 100644 --- a/Source/cmServer.cxx +++ b/Source/cmServer.cxx @@ -22,13 +22,14 @@ void on_signal(uv_signal_t* signal, int signum) { - auto conn = reinterpret_cast(signal->data); + auto conn = static_cast(signal->data); conn->OnSignal(signum); } static void on_walk_to_shutdown(uv_handle_t* handle, void* arg) { (void)arg; + assert(uv_is_closing(handle)); if (!uv_is_closing(handle)) { uv_close(handle, &cmEventBasedConnection::on_close); } @@ -58,6 +59,8 @@ cmServer::cmServer(cmConnection* conn, bool supportExperimental) cmServer::~cmServer() { + Close(); + for (cmServerProtocol* p : this->SupportedProtocols) { delete p; } @@ -409,7 +412,7 @@ void cmServer::StartShutDown() static void __start_thread(void* arg) { - auto server = reinterpret_cast(arg); + auto server = static_cast(arg); std::string error; bool success = server->Serve(&error); if (!success || error.empty() == false) { @@ -417,22 +420,19 @@ static void __start_thread(void* arg) } } -static void __shutdownThread(uv_async_t* arg) -{ - auto server = reinterpret_cast(arg->data); - on_walk_to_shutdown(reinterpret_cast(arg), nullptr); - server->StartShutDown(); -} - bool cmServerBase::StartServeThread() { ServeThreadRunning = true; - uv_async_init(&Loop, &this->ShutdownSignal, __shutdownThread); - this->ShutdownSignal.data = this; uv_thread_create(&ServeThread, __start_thread, this); return true; } +static void __shutdownThread(uv_async_t* arg) +{ + auto server = static_cast(arg->data); + server->StartShutDown(); +} + bool cmServerBase::Serve(std::string* errorMessage) { #ifndef NDEBUG @@ -443,14 +443,13 @@ bool cmServerBase::Serve(std::string* errorMessage) errorMessage->clear(); - uv_signal_init(&Loop, &this->SIGINTHandler); - uv_signal_init(&Loop, &this->SIGHUPHandler); + ShutdownSignal.init(Loop, __shutdownThread, this); - this->SIGINTHandler.data = this; - this->SIGHUPHandler.data = this; + SIGINTHandler.init(Loop, this); + SIGHUPHandler.init(Loop, this); - uv_signal_start(&this->SIGINTHandler, &on_signal, SIGINT); - uv_signal_start(&this->SIGHUPHandler, &on_signal, SIGHUP); + SIGINTHandler.start(&on_signal, SIGINT); + SIGHUPHandler.start(&on_signal, SIGHUP); OnServeStart(); @@ -473,7 +472,6 @@ bool cmServerBase::Serve(std::string* errorMessage) return false; } - ServeThreadRunning = false; return true; } @@ -487,15 +485,9 @@ void cmServerBase::OnServeStart() void cmServerBase::StartShutDown() { - if (!uv_is_closing( - reinterpret_cast(&this->SIGINTHandler))) { - uv_signal_stop(&this->SIGINTHandler); - } - - if (!uv_is_closing( - reinterpret_cast(&this->SIGHUPHandler))) { - uv_signal_stop(&this->SIGHUPHandler); - } + ShutdownSignal.reset(); + SIGINTHandler.reset(); + SIGHUPHandler.reset(); { cm::unique_lock lock(ConnectionsMutex); @@ -519,20 +511,27 @@ cmServerBase::cmServerBase(cmConnection* connection) { auto err = uv_loop_init(&Loop); (void)err; + Loop.data = this; assert(err == 0); AddNewConnection(connection); } -cmServerBase::~cmServerBase() +void cmServerBase::Close() { + if (Loop.data) { + if (ServeThreadRunning) { + this->ShutdownSignal.send(); + uv_thread_join(&ServeThread); + } - if (ServeThreadRunning) { - uv_async_send(&this->ShutdownSignal); - uv_thread_join(&ServeThread); + uv_loop_close(&Loop); + Loop.data = nullptr; } - - uv_loop_close(&Loop); +} +cmServerBase::~cmServerBase() +{ + Close(); } void cmServerBase::AddNewConnection(cmConnection* ownedConnection) @@ -562,6 +561,6 @@ void cmServerBase::OnDisconnect(cmConnection* pConnection) } if (Connections.empty()) { - StartShutDown(); + this->ShutdownSignal.send(); } } diff --git a/Source/cmServer.h b/Source/cmServer.h index 6e46f8c..ca37ce2 100644 --- a/Source/cmServer.h +++ b/Source/cmServer.h @@ -8,6 +8,8 @@ #include "cm_thread.hxx" #include "cm_uv.h" +#include "cmUVHandlePtr.h" + #include // IWYU pragma: keep #include #include @@ -58,7 +60,7 @@ public: virtual bool OnSignal(int signum); uv_loop_t* GetLoop(); - + void Close(); void OnDisconnect(cmConnection* pConnection); protected: @@ -67,7 +69,7 @@ protected: bool ServeThreadRunning = false; uv_thread_t ServeThread; - uv_async_t ShutdownSignal; + cm::uv_async_ptr ShutdownSignal; #ifndef NDEBUG public: // When the server starts it will mark down it's current thread ID, @@ -80,8 +82,8 @@ protected: uv_loop_t Loop; - uv_signal_t SIGINTHandler; - uv_signal_t SIGHUPHandler; + cm::uv_signal_ptr SIGINTHandler; + cm::uv_signal_ptr SIGHUPHandler; }; class cmServer : public cmServerBase diff --git a/Source/cmServerConnection.cxx b/Source/cmServerConnection.cxx index 44af75f..78c8f06 100644 --- a/Source/cmServerConnection.cxx +++ b/Source/cmServerConnection.cxx @@ -5,6 +5,9 @@ #include "cmConfigure.h" #include "cmServer.h" #include "cmServerDictionary.h" +#include "cm_uv.h" + +#include #ifdef _WIN32 #include "io.h" #else @@ -18,36 +21,34 @@ cmStdIoConnection::cmStdIoConnection( { } -void cmStdIoConnection::SetupStream(uv_stream_t*& stream, int file_id) +cm::uv_stream_ptr cmStdIoConnection::SetupStream(int file_id) { - assert(stream == nullptr); switch (uv_guess_handle(file_id)) { case UV_TTY: { - auto tty = new uv_tty_t(); - uv_tty_init(this->Server->GetLoop(), tty, file_id, file_id == 0); + cm::uv_tty_ptr tty; + tty.init(*this->Server->GetLoop(), file_id, file_id == 0, + static_cast(this)); uv_tty_set_mode(tty, UV_TTY_MODE_NORMAL); - stream = reinterpret_cast(tty); - break; + return std::move(tty); } case UV_FILE: if (file_id == 0) { - return; + return nullptr; } // Intentional fallthrough; stdin can _not_ be treated as a named // pipe, however stdout can be. CM_FALLTHROUGH; case UV_NAMED_PIPE: { - auto pipe = new uv_pipe_t(); - uv_pipe_init(this->Server->GetLoop(), pipe, 0); + cm::uv_pipe_ptr pipe; + pipe.init(*this->Server->GetLoop(), 0, + static_cast(this)); uv_pipe_open(pipe, file_id); - stream = reinterpret_cast(pipe); - break; + return std::move(pipe); } default: assert(false && "Unable to determine stream type"); - return; + return nullptr; } - stream->data = static_cast(this); } void cmStdIoConnection::SetServer(cmServerBase* s) @@ -57,14 +58,14 @@ void cmStdIoConnection::SetServer(cmServerBase* s) return; } - SetupStream(this->ReadStream, 0); - SetupStream(this->WriteStream, 1); + this->ReadStream = SetupStream(0); + this->WriteStream = SetupStream(1); } void shutdown_connection(uv_prepare_t* prepare) { cmStdIoConnection* connection = - reinterpret_cast(prepare->data); + static_cast(prepare->data); if (!uv_is_closing(reinterpret_cast(prepare))) { uv_close(reinterpret_cast(prepare), @@ -76,7 +77,7 @@ void shutdown_connection(uv_prepare_t* prepare) bool cmStdIoConnection::OnServeStart(std::string* pString) { Server->OnConnected(this); - if (this->ReadStream) { + if (this->ReadStream.get()) { uv_read_start(this->ReadStream, on_alloc_buffer, on_read); } else if (uv_guess_handle(0) == UV_FILE) { char buffer[1024]; @@ -94,44 +95,14 @@ bool cmStdIoConnection::OnServeStart(std::string* pString) return cmConnection::OnServeStart(pString); } -void cmStdIoConnection::ShutdownStream(uv_stream_t*& stream) -{ - if (!stream) { - return; - } - switch (stream->type) { - case UV_TTY: { - assert(!uv_is_closing(reinterpret_cast(stream))); - if (!uv_is_closing(reinterpret_cast(stream))) { - uv_close(reinterpret_cast(stream), - &on_close_delete); - } - break; - } - case UV_FILE: - case UV_NAMED_PIPE: { - assert(!uv_is_closing(reinterpret_cast(stream))); - if (!uv_is_closing(reinterpret_cast(stream))) { - uv_close(reinterpret_cast(stream), - &on_close_delete); - } - break; - } - default: - assert(false && "Unable to determine stream type"); - } - - stream = nullptr; -} - bool cmStdIoConnection::OnConnectionShuttingDown() { - if (ReadStream) { + if (ReadStream.get()) { uv_read_stop(ReadStream); + ReadStream->data = nullptr; } - ShutdownStream(ReadStream); - ShutdownStream(WriteStream); + this->ReadStream.reset(); cmEventBasedConnection::OnConnectionShuttingDown(); diff --git a/Source/cmServerConnection.h b/Source/cmServerConnection.h index 4ca908d..a70edb4 100644 --- a/Source/cmServerConnection.h +++ b/Source/cmServerConnection.h @@ -8,7 +8,7 @@ #include "cmConnection.h" #include "cmPipeConnection.h" -#include "cm_uv.h" +#include "cmUVHandlePtr.h" class cmServerBase; @@ -46,8 +46,8 @@ public: bool OnServeStart(std::string* pString) override; private: - void SetupStream(uv_stream_t*& stream, int file_id); - void ShutdownStream(uv_stream_t*& stream); + cm::uv_stream_ptr SetupStream(int file_id); + cm::uv_stream_ptr ReadStream; }; /*** -- cgit v0.12