From e49a925a4055f27ee727377d7bea2423e33ba511 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 28 Feb 2024 13:34:10 -0500 Subject: [PATCH 1/7] doc: Add comments to mp.Context PassField function on mp.Context.thread lookup --- include/mp/proxy-types.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/include/mp/proxy-types.h b/include/mp/proxy-types.h index 8236b0c9..3ef901ca 100644 --- a/include/mp/proxy-types.h +++ b/include/mp/proxy-types.h @@ -157,9 +157,15 @@ auto PassField(Priority<1>, TypeList<>, ServerContext& server_context, const Fn& } }); + // Lookup Thread object specified by the client. The specified thread should + // be a local Thread::Server object, but it needs to be looked up + // asynchronously with getLocalServer(). auto thread_client = context_arg.getThread(); return JoinPromises(server.m_context.connection->m_threads.getLocalServer(thread_client) .then([&server, invoke, req](const kj::Maybe& perhaps) { + // Assuming the thread object is found, pass it a pointer to the + // `invoke` lambda above which will invoke the function on that + // thread. KJ_IF_MAYBE(thread_server, perhaps) { const auto& thread = static_cast&>(*thread_server); @@ -174,6 +180,7 @@ auto PassField(Priority<1>, TypeList<>, ServerContext& server_context, const Fn& throw std::runtime_error("invalid thread handle"); } }), + // Wait for the invocation to finish before returning to the caller. kj::mv(future.promise)); } From a1dfb0bab4778e8382848f6299fecb20afc9383f Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 28 Feb 2024 14:51:44 -0500 Subject: [PATCH 2/7] doc: Add comments to mp.Context PassField function on updating g_thread_context --- include/mp/proxy-types.h | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/include/mp/proxy-types.h b/include/mp/proxy-types.h index 3ef901ca..046f2142 100644 --- a/include/mp/proxy-types.h +++ b/include/mp/proxy-types.h @@ -126,6 +126,23 @@ auto PassField(Priority<1>, TypeList<>, ServerContext& server_context, const Fn& Context::Reader context_arg = Accessor::get(params); ServerContext server_context{server, call_context, req}; { + // Before invoking the function, store a reference to the + // callbackThread provided by the client in the + // thread_local.request_threads map. This way, if this + // server thread needs to execute any RPCs that call back to + // the client, they will happen on the same client thread + // that is waiting for this function, just like what would + // happen if this were a normal function call made on the + // local stack. + // + // If the request_threads map already has an entry for this + // connection, it will be left unchanged, and it indicates + // that the current thread is an RPC client thread which is + // in the middle of an RPC call, and the current RPC call is + // a nested call from the remote thread handling that RPC + // call. In this case, the callbackThread value should point + // to the same thread already in the map, so there is no + // need to update the map. auto& request_threads = g_thread_context.request_threads; auto request_thread = request_threads.find(server.m_context.connection); if (request_thread == request_threads.end()) { @@ -136,8 +153,11 @@ auto PassField(Priority<1>, TypeList<>, ServerContext& server_context, const Fn& /* destroy_connection= */ false)) .first; } else { - // If recursive call, avoid remove request_threads map - // entry in KJ_DEFER below. + // The requests_threads map already has an entry for + // this connection, so this must be a recursive call. + // Avoid modifying the map in this case by resetting the + // request_thread iterator, so the KJ_DEFER statement + // below doesn't do anything. request_thread = request_threads.end(); } KJ_DEFER(if (request_thread != request_threads.end()) request_threads.erase(request_thread)); From 2098ae1b186f96cd14b77eee7f5cc8504acba9a7 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 28 Feb 2024 14:51:59 -0500 Subject: [PATCH 3/7] doc: Add comment on serverInvoke ReplaceVoid usage --- include/mp/proxy-types.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/include/mp/proxy-types.h b/include/mp/proxy-types.h index 046f2142..7449cb2d 100644 --- a/include/mp/proxy-types.h +++ b/include/mp/proxy-types.h @@ -1553,6 +1553,13 @@ kj::Promise serverInvoke(Server& server, CallContext& call_context, Fn fn) using ServerContext = ServerInvokeContext; using ArgList = typename ProxyClientMethodTraits::Params; ServerContext server_context{server, call_context, req}; + // ReplaceVoid is used to support fn.invoke implementations that + // execute asynchronously and return promises, as well as + // implementations that execute synchronously and return void. The + // invoke function will be synchronous by default, but asynchronous if + // an mp.Context argument is passed, and the mp.Context PassField + // overload returns a promise executing the request in a worker thread + // and waiting for it to complete. return ReplaceVoid([&]() { return fn.invoke(server_context, ArgList()); }, [&]() { return kj::Promise(kj::mv(call_context)); }) .then([&server, req](CallContext call_context) { From e99c0b75648aa2928c16f4de575786f66f785180 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 28 Feb 2024 15:36:13 -0500 Subject: [PATCH 4/7] doc: Document clientInvoke/serverInvoke functions Show how they are invoked by generated code --- include/mp/proxy-types.h | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/include/mp/proxy-types.h b/include/mp/proxy-types.h index 7449cb2d..f343ecf7 100644 --- a/include/mp/proxy-types.h +++ b/include/mp/proxy-types.h @@ -1450,6 +1450,15 @@ void serverDestroy(Server& server) server.m_context.connection->m_loop.log() << "IPC server destroy" << typeid(server).name(); } +//! Entry point called by generated client code that looks like: +//! +//! ProxyClient::M0::Result ProxyClient::methodName(M0::Param<0> arg0, M0::Param<1> arg1) { +//! typename M0::Result result; +//! clientInvoke(*this, &InterfaceName::Client::methodNameRequest, MakeClientParam<...>(arg0), MakeClientParam<...>(arg1), MakeClientParam<...>(result)); +//! return result; +//! } +//! +//! Ellipses above are where generated Accessor<> type declarations are inserted. template void clientInvoke(ProxyClient& proxy_client, const GetRequest& get_request, FieldObjs&&... fields) { @@ -1538,6 +1547,13 @@ auto ReplaceVoid(Fn&& fn, Ret&& ret) -> extern std::atomic server_reqs; +//! Entry point called by generated server code that looks like: +//! +//! kj::Promise ProxyServer::methodName(CallContext call_context) { +//! return serverInvoke(*this, call_context, MakeServerField<0, ...>(MakeServerField<1, ...>(Make(ServerCall())))); +//! } +//! +//! Ellipses above are where generated Accessor<> type declarations are inserted. template kj::Promise serverInvoke(Server& server, CallContext& call_context, Fn fn) { From 78c7dd0be54219c4f5ac3e3ea285254b8f7eafce Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 28 Feb 2024 17:29:38 -0500 Subject: [PATCH 5/7] doc: Document ProxyClient construct/destroy methods --- include/mp/proxy.h | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/include/mp/proxy.h b/include/mp/proxy.h index 87416d27..9528c590 100644 --- a/include/mp/proxy.h +++ b/include/mp/proxy.h @@ -60,8 +60,35 @@ class ProxyClientBase : public Impl_ ProxyClientBase(typename Interface::Client client, Connection* connection, bool destroy_connection); ~ProxyClientBase() noexcept; - // Methods called during client construction/destruction that can optionally - // be defined in capnp interface to trigger the server. + // construct/destroy methods called during client construction/destruction + // that can optionally be defined in capnp interfaces to invoke code on the + // server when proxy client objects are created and destroyed. + // + // The construct() method is not generally very useful, but can be used to + // run custom code on the server automatically when a ProxyClient client is + // constructed. The only current use is adding a construct method to Init + // interfaces that is called automatically on construction, so client and + // server exchange ThreadMap references and set Connection::m_thread_map + // values as soon as the Init client is created. + // + // construct @0 (threadMap: Proxy.ThreadMap) -> (threadMap: Proxy.ThreadMap); + // + // But construct() is not necessary for this, thread maps could be passed + // through a normal method that is just called explicitly rather than + // implicitly. + // + // The destroy() method is more generally useful than construct(), because + // it ensures that the server object will be destroyed synchronously before + // the client destructor returns, instead of asynchronously at some + // unpredictable time after the client object is already destroyed and + // client code has moved on. If the destroy method accepts a Context + // parameter like: + // + // destroy @0 (context: Proxy.Context) -> (); + // + // then it will also ensure that the destructor runs on the same thread the + // client used to make other RPC calls, instead of running on the server + // EventLoop thread and possibly blocking it. void construct() {} void destroy() {} From d4d9f931756242a29f41b043286b834813caf439 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 28 Feb 2024 17:30:50 -0500 Subject: [PATCH 6/7] doc: Document FunctionTraits/ProxyMethodTraits classes --- include/mp/proxy.h | 53 +++++++++++++++++++++++++++++++++++----------- 1 file changed, 41 insertions(+), 12 deletions(-) diff --git a/include/mp/proxy.h b/include/mp/proxy.h index 9528c590..39722ec0 100644 --- a/include/mp/proxy.h +++ b/include/mp/proxy.h @@ -144,12 +144,26 @@ struct ProxyServerCustom : public ProxyServerBase using ProxyServerBase::ProxyServerBase; }; -//! Function traits class used to get method parameter and result types in generated ProxyClient implementations from -//! proxy-codegen.cpp. +//! Function traits class used to get method parameter and result types, used in +//! generated ProxyClient and ProxyServer classes produced by gen.cpp to get C++ +//! method type information. The generated code accesses these traits via +//! intermediate ProxyClientMethodTraits and ProxyServerMethodTraits classes, +//! which it is possible to specialize to change the way method arguments and +//! return values are handled. +//! +//! Fields of the trait class are: +//! +//! Params - TypeList of C++ ClassName::methodName parameter types +//! Result - Return type of ClassName::method +//! Param - helper to access individual parameters by index number. +//! Fields - helper alias that appends Result type to the Params typelist if +//! it not void. template struct FunctionTraits; -//! Specialization of above to extract result and params types. +//! Specialization of above extracting result and params types assuming the +//! template argument is a pointer-to-method type, +//! decltype(&ClassName::methodName) template struct FunctionTraits<_Result (_Class::*const)(_Params...)> { @@ -161,16 +175,20 @@ struct FunctionTraits<_Result (_Class::*const)(_Params...)> typename std::conditional::value, Params, TypeList<_Params..., _Result>>::type; }; -//! Traits class for a method specialized by method parameters. +//! Traits class for a proxy method, providing the same +//! Params/Result/Param/Fields described in the FunctionTraits class above, plus +//! an additional invoke() method that calls the C++ method which is being +//! proxied, forwarding any arguments. //! -//! Param and Result typedefs can be customized to adjust parameter and return types on client side. +//! The template argument should be the InterfaceName::MethodNameParams class +//! (generated by Cap'n Proto) associated with the method. //! -//! Invoke method customized to adjust parameter and return types on server side. -//! -//! Normal method calls go through the ProxyMethodTraits struct specialization -//! below, not this default struct, which is only used if there is no -//! ProxyMethod::impl method pointer, which is only true for construct/destroy -//! methods. +//! Note: The class definition here is just the fallback definition used when +//! the other specialization below doesn't match. The fallback is only used for +//! capnp methods which do not have corresponding C++ methods, which in practice +//! is just the two special construct() and destroy() methods described in \ref +//! ProxyClientBase. These methods don't have any C++ parameters or return +//! types, so the trait information below reflects that. template struct ProxyMethodTraits { @@ -184,7 +202,18 @@ struct ProxyMethodTraits } }; -//! Specialization of above. +//! Specialization of above for proxy methods that have a +//! ProxyMethod::impl pointer-to-method +//! constant defined by generated code. This includes all functions defined in +//! the capnp interface except any construct() or destroy() methods, that are +//! assumed not to correspond to real member functions in the C++ class, and +//! will use the fallback traits definition above. The generated code this +//! specialization relies on looks like: +//! +//! struct ProxyMethod +//! { +//! static constexpr auto impl = &ClassName::methodName; +//! }; template struct ProxyMethodTraits::impl)>> : public FunctionTraits::impl)> From 537c645652cb98a641bb7e40ff541c89bf4ec471 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 13 Jun 2024 07:59:53 -0400 Subject: [PATCH 7/7] doc: Improve ProxyServerCustom class documentation Add note about cleaning up custom state asynchronously, after a recently fixed bug where there was a wallet deadlock when the node connection was broken, caused by the ProxyServerCustom destructor trying to delete the wallet CScheduler object synchronously (on the IPC EventLoop thread) while a scheduled task was trying to make an Chain isReadyToBroadcast() IPC call. --- include/mp/proxy.h | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/include/mp/proxy.h b/include/mp/proxy.h index 39722ec0..1ee1c753 100644 --- a/include/mp/proxy.h +++ b/include/mp/proxy.h @@ -136,8 +136,26 @@ struct ProxyServerBase : public virtual Interface_::Server ProxyContext m_context; }; -//! Customizable (through template specialization) base class used in generated ProxyServer implementations from -//! proxy-codegen.cpp. +//! Customizable (through template specialization) base class which ProxyServer +//! classes produced by generated code will inherit from. The default +//! specialization of this class just inherits from ProxyServerBase, but custom +//! specializations can be defined to control ProxyServer behavior. +//! +//! Specifically, it can be useful to specialize this class to add additional +//! state to ProxyServer classes, for example to cache state between IPC calls. +//! If this is done, however, care should be taken to ensure that the extra +//! state can be destroyed without blocking, because ProxyServer destructors are +//! called from the EventLoop thread, and if they block, it could deadlock the +//! program. One way to do avoid blocking is to clean up the state by pushing +//! cleanup callbacks to the m_context.cleanup list, which run after the server +//! m_impl object is destroyed on the same thread destroying it (which will +//! either be an IPC worker thread if the ProxyServer is being explicitly +//! destroyed by a client calling a destroy() method with a Context argument and +//! Context.thread value set, or the temporary EventLoop::m_async_thread used to +//! run destructors without blocking the event loop when no-longer used server +//! objects are garbage collected by Cap'n Proto.) Alternately, if cleanup needs +//! to run before m_impl is destroyed, the specialization can override +//! invokeDestroy and destructor methods to do that. template struct ProxyServerCustom : public ProxyServerBase {