From f436f9eeae02615ca4c1f4db30796f394396993d Mon Sep 17 00:00:00 2001 From: Yuri Goldfeld Date: Thu, 12 Dec 2024 17:35:20 -0800 Subject: [PATCH 1/2] Comment and/or doc changes. --- .../shm/arena_lend/jemalloc/shm_session.hpp | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/ipc/session/standalone/shm/arena_lend/jemalloc/shm_session.hpp b/src/ipc/session/standalone/shm/arena_lend/jemalloc/shm_session.hpp index 6b7eb1a..b355c63 100644 --- a/src/ipc/session/standalone/shm/arena_lend/jemalloc/shm_session.hpp +++ b/src/ipc/session/standalone/shm/arena_lend/jemalloc/shm_session.hpp @@ -131,11 +131,14 @@ class Shm_session : * #borrow_object. The serialization contents include information on how to deserialize the object and not * contents of the object itself. * + * @warning Be ready for this method returning empty blob. Assuming you provided proper inputs, this indicates session + * is hosed; in particular no further lending/borrowing is likely to work. Most likely the opposing process + * is down or chose to close the session. + * * @param object The object to be lent. * * @return If successful, the serialized handle of the object, which should be transmitted by the caller (lender) - * and deserialized by the borrower. If unsuccessful, an empty blob. This would only fail if the object - * was previously registered. + * and deserialized by the borrower. If unsuccessful, an empty blob. */ template Blob lend_object(const std::shared_ptr& object); @@ -145,11 +148,14 @@ class Shm_session : * The caller must have prior knowledge of what type the serialized object is. When the object is released (i.e., * shared pointer handle count drops to zero), the object will be deregistered and communicated back to the lender. * + * @warning Be ready for this method returning null. Assuming you provided proper inputs, this indicates the session + * is hosed; in particular no further lending/borrowing is likely to work. Most likely the opposing process + * is down or chose to close the session. + * * @tparam T The object type that is being borrowed. * @param serialized_object A serialized handle of an object, which was previously serialized via #lend_object. * - * @return If successful, the (deserialized) object. If unsuccessful, a null object. Failure would only occur if - * the size of the serialized object is insufficient or the (deserialized) object was previously registered. + * @return If successful, the (deserialized) object. If unsuccessful, a null object. */ template std::shared_ptr borrow_object(const Blob& serialized_object); From fa75c378bcaa5a63ae4a4fa99288121d108470ab Mon Sep 17 00:00:00 2001 From: Yuri Goldfeld Date: Thu, 12 Dec 2024 17:59:12 -0800 Subject: [PATCH 2/2] In the SHM-jemalloc session glue layer account for the realistic possibility that when one uses session.lend/borrow_object() to transmit native structures residing in SHM, those operations can fail due to session being down at that time. More or less that means (1) correcting a few log messages and comments and (2) do not "helpfully" assert on things that might not always be true. --- .../jemalloc/server_session_impl.hpp | 2 +- .../shm/arena_lend/jemalloc/session_impl.hpp | 23 +++++++------------ .../arena_lend/jemalloc/server_session.hpp | 4 ++++ .../shm/arena_lend/jemalloc/session.hpp | 16 +++++++++++-- 4 files changed, 27 insertions(+), 18 deletions(-) diff --git a/src/ipc/session/detail/shm/arena_lend/jemalloc/server_session_impl.hpp b/src/ipc/session/detail/shm/arena_lend/jemalloc/server_session_impl.hpp index 4c55cef..6a113f9 100644 --- a/src/ipc/session/detail/shm/arena_lend/jemalloc/server_session_impl.hpp +++ b/src/ipc/session/detail/shm/arena_lend/jemalloc/server_session_impl.hpp @@ -230,7 +230,7 @@ void CLASS_JEM_SRV_SESSION_IMPL::async_accept_log_in /* Do our extra stuff on top of the base vanilla Server_session_impl. That is Base::session_shm() must * work (return non-null), as must app_shm() (ditto, cached in this->m_app_shm); plus lend_object()ing * objects constructed from either must work, and borrow_object()ing from anything the opposing process - * transmits must work. The lend/borrow stuff will work if and only if session_shm() and app_shm() have + * transmits must work. The lend/borrow stuff will work only if session_shm() and app_shm() have * been registered with the lend/borrow engine, namely shm_session(). * Incidentally Base::shm_session() must also be available (return non-null), in case the user wants to * access that guy directly. diff --git a/src/ipc/session/detail/shm/arena_lend/jemalloc/session_impl.hpp b/src/ipc/session/detail/shm/arena_lend/jemalloc/session_impl.hpp index 2508a13..bee3b2e 100644 --- a/src/ipc/session/detail/shm/arena_lend/jemalloc/session_impl.hpp +++ b/src/ipc/session/detail/shm/arena_lend/jemalloc/session_impl.hpp @@ -417,8 +417,10 @@ Error_code CLASS_JEM_SESSION_IMPL::init_shm * which means, similarly to the above incoming-direction handler do the following. * * Same comment and @todo(s) apply as in the handler above. This is a little different, as it's a send error, - * which wouldn't be reported as a channel error; but still we figure it something went this wrong with this - * channel, the session master channel will be hosed too. And even that caveat (again) only matters + * which wouldn't be reported as a channel error; but still we figure if something went this wrong with this + * channel, the session master channel will be hosed too. Plus, any attempt to m_shm_session->lend_arena() + * (by us below) or ->lend_object() or ->borrow_object() (when transmitting SHM-allocated objects process-to-process + * in the session) will fail (returning special failure values). And even those caveats (again) only matter * after *setup_done; before *setup_done we will absolutely catch the problem before exiting the present method. */ Base::async_worker()->post([this, setup_done = std::move(setup_done), err_code]() { @@ -426,7 +428,8 @@ Error_code CLASS_JEM_SESSION_IMPL::init_shm { FLOW_LOG_WARNING("Session [" << * this << "]: Internal-use (for SHM) channel reported outgoing-direction " "error [" << err_code << "] [" << err_code.message() << "]. This occured after SHM-setup; " - "almost certainly the session master channel will catch a problem or has caught it; " + "almost certainly the session master channel and/or attempts to lend/borrow " + "will catch a problem or have caught it; " "session will be hosed, or session opening will fail, depending on the situation."); } else @@ -559,12 +562,7 @@ template typename CLASS_JEM_SESSION_IMPL::Blob CLASS_JEM_SESSION_IMPL::lend_object(const typename Arena::template Handle& handle) { - auto blob = shm_session()->template lend_object(handle); - - // @todo Maybe we should just return empty Blob? For now doing this: SHM-classic will assert() too. Reconsider both. - assert((!blob.empty()) && "Most likely `handle` did not come validly from one of `*this`-owned `Arena`s."); - - return blob; + return shm_session()->template lend_object(handle); } TEMPLATE_JEM_SESSION_IMPL @@ -572,12 +570,7 @@ template typename CLASS_JEM_SESSION_IMPL::Arena::template Handle CLASS_JEM_SESSION_IMPL::borrow_object(const Blob& serialization) { - const auto obj = shm_session()->template borrow_object(serialization); - - // @todo Maybe we should just return nullptr? For now doing this: SHM-classic will assert() too. Reconsider both. - assert(obj && "Most likely `serialization` is invalid."); - - return obj; + return shm_session()->template borrow_object(serialization); } TEMPLATE_JEM_SESSION_IMPL diff --git a/src/ipc/session/shm/arena_lend/jemalloc/server_session.hpp b/src/ipc/session/shm/arena_lend/jemalloc/server_session.hpp index cfeb7a6..05480c7 100644 --- a/src/ipc/session/shm/arena_lend/jemalloc/server_session.hpp +++ b/src/ipc/session/shm/arena_lend/jemalloc/server_session.hpp @@ -70,6 +70,10 @@ namespace ipc::session::shm::arena_lend::jemalloc * arena-sharing SHM-provider by definition both sides allocate and thus write to the same SHM pool(s). * However this comes at the expense of safety. Further discussion is up to ipc::shm docs.) * + * @note Be ready for Session_mv::lend_object() to return an empty blob and Session_mv::borrow_object() to return + * a null pointer, both indicating an error: the session is hosed (opposing process likely down). See their + * doc headers for a bit more info on this. + * * By using `shm::stl::Arena_activator` (alias shm::arena_lend::jemalloc::Ipc_arena_activator) and * #Allocator (alias shm::arena_lend::jemalloc::Ipc_arena_allocator) * it is possible to construct and transmit not just POD (Plain Old Datatype) objects but combinations of those diff --git a/src/ipc/session/shm/arena_lend/jemalloc/session.hpp b/src/ipc/session/shm/arena_lend/jemalloc/session.hpp index 15239e4..886a439 100644 --- a/src/ipc/session/shm/arena_lend/jemalloc/session.hpp +++ b/src/ipc/session/shm/arena_lend/jemalloc/session.hpp @@ -213,6 +213,14 @@ class Session_mv : * However, if your code specifically counts on `*this` being a shm::arena_lend::jemalloc::Server_session, then * it is not wrong to rely on this knowledge. * + * ### Possibility of error ### + * This method returns an `.empty()` blob in the event of error. Assuming you used it with proper inputs, this + * indicates the session is hosed -- most likely the opposing process is down -- and therefore your code should + * be ready for this absolutely possible eventuality. The session-hosed condition *will* shortly be indicated via + * session on-error handler as well. Informally we suggest handling the first sign of the session going down (whether + * this returning empty, borrow_object() returning null, or the session on-error handler firing) uniformly by + * abandoning the entire session ASAP. + * * @tparam T * See #Arena. * @param handle @@ -220,7 +228,7 @@ class Session_mv : * unspecified custom deleter logic attached. The #Arena instance can be session_shm(), * Server_session_mv::app_shm(), or a non ipc::session-managed custom #Arena, as long as it has been * registered via `shm_session()->lend_arena()`. - * @return See above. Never `.empty()`. + * @return See above. On success, non-empty; otherwise an `.empty()` blob. */ template Blob lend_object(const typename Arena::template Handle& handle); @@ -229,11 +237,15 @@ class Session_mv : * Completes the cross-process operation begun by oppsing Session_mv::lend_object() that returned `serialization`; * to be invoked in the intended new owner process which is operating `*this`. * + * ### Possibility of error ### + * Indicated by this returning null, the remarks in the lend_object() doc header "Possibility of error" section + * apply here equally. Reminder: be ready for this to return null; it can absolutely happen. + * * @tparam T * See lend_object(). * @param serialization * Value, not `.empty()`, returned by opposing lend_object() and transmitted bit-for-bit to this process. - * @return See above. Never null. + * @return See above. On success, non-null pointer; otherwise a null pointer. */ template typename Arena::template Handle borrow_object(const Blob& serialization);