Skip to content

Commit

Permalink
Merge pull request #81 from Flow-IPC/polish
Browse files Browse the repository at this point in the history
Fixed meaty SHM-jemalloc-integration bug: session-obj A must not be destroyed along with its arena, until opposing B enters dtor; else B user may access jemalloc-deinitialized memory (found via perf_demo). / Misc minor fixes.
  • Loading branch information
ygoldfeld authored Mar 6, 2024
2 parents a9fbafb + 1afe65f commit 0afb13d
Show file tree
Hide file tree
Showing 6 changed files with 5 additions and 34 deletions.
2 changes: 1 addition & 1 deletion flow
Submodule flow updated 1008 files
2 changes: 1 addition & 1 deletion ipc_shm
3 changes: 0 additions & 3 deletions test/suite/perf_demo/main_cli.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -427,9 +427,6 @@ void run_capnp_zero_cpy([[maybe_unused]] flow::log::Logger* logger_ptr, Channel_
g_capnp_zero_cpy_rtt = m_timer->since_start().m_values[size_t(Clock_type::S_REAL_HI_RES)];

rsp.reset();
FLOW_LOG_INFO("> Signaling server we are done; they can blow everything away now.");

m_chan.send(m_chan.create_msg());
g_asio.stop();
} // on_complete_response()
}; // class Algo
Expand Down
28 changes: 1 addition & 27 deletions test/suite/perf_demo/main_srv.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -498,33 +498,7 @@ void run_capnp_zero_copy(flow::log::Logger* logger_ptr, Channel_struc* chan_ptr,
FLOW_LOG_INFO("> Sending get-cache (quite large) response.");
m_chan.send(m_capnp_msg, req.get());
FLOW_LOG_INFO("= Done.");
m_capnp_msg = {};

/* There's a little subtlety here; basically everything is cool with the backing memory, until
* the Session_server dtor runs, specifically in SHM-jemalloc's case; as it'll deinitialize jemalloc
* and... stuff; the details don't matter (actually the fact that SHM-classic doesn't have this to worry about
* is merely an internal property; formally one should still not access SHM-backed items once session-server
* goes away -- even with SHM-classic). What matters = the principles explained in
* "Sessions: Teardown; Organizing Your Code" in the guided Manual; which is that when a session-hosing
* error is reported, one should nullify any SHM-backed objects related to that session; then sometime later
* (usually soon but w/e) destroy the Session object (have its dtor run). If it's done in that order,
* and it's not a crash/zombie situation on the other side, then everything will be safe. (If it's a crash/zombie
* situation, then one should still follow that procedure; and there's a good -- as good as possible -- chance
* one would escape bad consequences.) As we explained in the "disclaimer" comment at the top: we didn't
* arrange this app in that nice organized way (for reasons); and our session and channel error handlers
* literally just no-op (see above). It is OK though; we control the order of things; all we need to do
* is nullify SHM-backed stuff before server goes down. That's why we let the client do that and then
* send us a message after that... and then we stop. To be clear: This isn't unsafe; it is safe and formally
* at that. Just, don't do this kind of stuff in serious applications.
*
* Also one should (not "must") use .async_end_sending() before Channel dtor. Again though... doesn't matter for
* us! But serious apps should do all the good stuff as recommended. */

FLOW_LOG_INFO("< Expecting client to signal they are done; so we can blow everything away.");
req.reset();
m_chan.expect_msg(Channel_struc::Msg_which_in::GET_CACHE_REQ, &req,
[&](auto&&) { g_asio.stop(); });
if (req) { g_asio.stop(); }
g_asio.stop();

/* The .stop() is needed here, because struc::Channel is always reading all internally incoming messages ASAP;
* so it always has an .async_wait() outstanding. Hence the .run() never runs out of work, unless we
Expand Down

0 comments on commit 0afb13d

Please sign in to comment.