Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion include/mp/proxy-io.h
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,8 @@ class EventLoop
//! other IPC calls.
void startAsyncThread(std::unique_lock<std::mutex>& lock);

//! Add/remove remote client reference counts.
//! Add/remove remote client reference counts. Can use EventLoopRef
//! below to avoid calling these directly.
void addClient(std::unique_lock<std::mutex>& lock);
bool removeClient(std::unique_lock<std::mutex>& lock);
//! Check if loop should exit.
Expand Down
21 changes: 21 additions & 0 deletions include/mp/proxy.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include <mp/util.h>

#include <array>
#include <cassert>
#include <functional>
#include <list>
#include <stddef.h>
Expand Down Expand Up @@ -47,6 +48,26 @@ inline void CleanupRun(CleanupList& fns) {
}
}

//! Event loop smart pointer automatically calling addClient and removeClient.
//! If a lock pointer argument is passed, the specified lock will be used,
//! otherwise EventLoop::m_mutex will be locked when needed.
class EventLoopRef
{
public:
explicit EventLoopRef(EventLoop& loop, std::unique_lock<std::mutex>* lock = nullptr);
EventLoopRef(EventLoopRef&& other) noexcept : m_loop(other.m_loop) { other.m_loop = nullptr; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell it is correct to not copy over the lock from the moved object here as long as the moved object has no lock. Should that be asserted?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #160 (comment)

In commit "proxy-io.h: Add EventLoopRef RAII class handle addClient/removeClient refcounting" (9aaeec3)

As far as I can tell it is correct to not copy over the lock from the moved object here as long as the moved object has no lock. Should that be asserted?

It's kind of a corner case, and I think either behavior could be useful here. The point of this constructor is to be able to efficiently move a reference count from one owner to another would pointlessly incrementing & decrement the count when creating one reference and dropping the other.

I feel like it probably would not be useful to copy the m_lock pointer when doing that because practical effect of copying would be to make the new EventLoopRef object destructor skip locking when it is destroyed (it would assert m_lock is locked instead).

On the other hand the default move-constructor would copy the m_lock value so maybe it is less surprising to follow the default behavior. And if the caller really wanted m_lock to be null after moving they could set it to null themselves.

So this is a good catch and I think I'd now lean towards adding m_lock{other.m_lock} but I don't think the difference should matter in practice.

More broadly I was also thinking about whether I could split EventLoopRef into a superclass without an m_lock member and a subclass with one to be able to get rid of the relock option added in ea38392 which requires turning off thread safety analysis in the reset function. At the moment this class seems good at preventing reference counting bugs because it guarantees reference counts will always be decremented if they are incremented, but it doesn't have very easy to understand locking semantics so that's probably an area for improvement.

EventLoopRef(const EventLoopRef&) = delete;
EventLoopRef& operator=(const EventLoopRef&) = delete;
EventLoopRef& operator=(EventLoopRef&&) = delete;
~EventLoopRef() { reset(); }
EventLoop& operator*() const { assert(m_loop); return *m_loop; }
EventLoop* operator->() const { assert(m_loop); return m_loop; }
bool reset();

EventLoop* m_loop{nullptr};
std::unique_lock<std::mutex>* m_lock{nullptr};
};

//! Context data associated with proxy client and server classes.
struct ProxyContext
{
Expand Down
15 changes: 15 additions & 0 deletions include/mp/util.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include <tuple>
#include <type_traits>
#include <utility>
#include <variant>
#include <vector>

namespace mp {
Expand Down Expand Up @@ -130,6 +131,20 @@ const char* TypeName()
return short_name ? short_name + 1 : display_name;
}

//! Convenient wrapper around std::variant<T*, T>
template <typename T>
struct PtrOrValue {
std::variant<T*, T> data;

template <typename... Args>
PtrOrValue(T* ptr, Args&&... args) : data(ptr ? ptr : std::variant<T*, T>{std::in_place_type<T>, std::forward<Args>(args)...}) {}

T& operator*() { return data.index() ? std::get<T>(data) : *std::get<T*>(data); }
T* operator->() { return &**this; }
T& operator*() const { return data.index() ? std::get<T>(data) : *std::get<T*>(data); }
T* operator->() const { return &**this; }
};

//! Analog to std::lock_guard that unlocks instead of locks.
template <typename Lock>
struct UnlockGuard
Expand Down
17 changes: 17 additions & 0 deletions src/mp/proxy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,23 @@ void LoggingErrorHandler::taskFailed(kj::Exception&& exception)
m_loop.log() << "Uncaught exception in daemonized task.";
}

EventLoopRef::EventLoopRef(EventLoop& loop, std::unique_lock<std::mutex>* lock) : m_loop(&loop), m_lock(lock)
{
auto loop_lock{PtrOrValue{m_lock, m_loop->m_mutex}};
m_loop->addClient(*loop_lock);
}

bool EventLoopRef::reset()
{
bool done = false;
if (auto* loop{m_loop}) {
m_loop = nullptr;
auto loop_lock{PtrOrValue{m_lock, loop->m_mutex}};
done = loop->removeClient(*loop_lock);
}
return done;
}

Connection::~Connection()
{
// Shut down RPC system first, since this will garbage collect Server
Expand Down