github-actions[bot] commented on code in PR #68942:
URL: https://github.com/apache/doris/pull/68942#discussion_r4255934340
##########
be/src/runtime/workload_management/resource_context.cpp:
##########
@@ -17,16 +17,137 @@
#include "runtime/workload_management/resource_context.h"
+#include <fmt/format.h>
#include <gen_cpp/FrontendService_types.h>
#include <gen_cpp/data.pb.h>
#include <glog/logging.h>
+#include <cstdint>
+#include <mutex>
+#include <unordered_set>
+#include <vector>
+
+#include "common/check.h"
+#include "common/config.h"
+#include "runtime/memory/mem_tracker_limiter.h"
#include "runtime/workload_group/workload_group.h"
#include "runtime/workload_management/query_task_controller.h"
+#include "util/pretty_printer.h"
#include "util/time.h"
+#include "util/uid_util.h"
namespace doris {
+namespace {
+
+// The resource contexts created while memleak-check mode is on. Their
addresses are kept
+// disguised (bitwise inverted), so that LeakSanitizer does not take the
registry for a
+// reference: a context that nothing else references must still be reported as
leaked.
+struct LiveResourceContexts {
+ std::mutex lock;
+ std::unordered_set<uintptr_t> disguised_contexts;
+};
+
+LiveResourceContexts& live_resource_contexts() {
+ // Never destroyed, so that a context released during static destruction
can still
+ // unregister itself.
+ static auto* live = new LiveResourceContexts();
+ return *live;
+}
+
+uintptr_t disguise(const ResourceContext* context) {
+ return ~reinterpret_cast<uintptr_t>(context);
+}
+
+ResourceContext* undisguise(uintptr_t disguised_context) {
+ return reinterpret_cast<ResourceContext*>(~disguised_context);
+}
+
+} // namespace
+
+void ResourceContext::_track_if_leak_check_enabled() {
+ if (!config::enable_graceful_exit_check) {
+ return;
+ }
+ auto& live = live_resource_contexts();
+ std::lock_guard<std::mutex> l(live.lock);
+ live.disguised_contexts.insert(disguise(this));
+ _tracked = true;
+}
+
+ResourceContext::~ResourceContext() {
+ if (_tracked) {
+ auto& live = live_resource_contexts();
+ std::lock_guard<std::mutex> l(live.lock);
+ DORIS_CHECK(live.disguised_contexts.erase(disguise(this)) == 1);
+ }
+}
+
+std::string ResourceContext::describe_live_contexts(size_t max_contexts) {
+ // Meant for shutdown, when no context is being created any more: a
context that is still
+ // inside std::make_shared has not been handed its weak_from_this() yet.
+ //
+ // Describe the contexts after the registry is unlocked, keeping each one
alive meanwhile
+ // with a reference of our own. A context whose last reference is being
released right now
+ // cannot be referenced any more and is left out.
+ std::vector<std::shared_ptr<ResourceContext>> contexts;
+ {
+ auto& live = live_resource_contexts();
+ std::lock_guard<std::mutex> l(live.lock);
+ contexts.reserve(live.disguised_contexts.size());
+ for (uintptr_t disguised_context : live.disguised_contexts) {
+ if (auto reference =
undisguise(disguised_context)->weak_from_this().lock()) {
Review Comment:
[P2] Synchronize context publication before taking the shutdown snapshot
`ExecEnv::destroy()` does not ensure that all context creators have stopped:
`CompactionAction::_handle_run_compaction()` detaches manual base/cumulative
compaction and waits only two seconds. A worker already owning its tablet and
memory tracker can be delayed immediately before
`SCOPED_ATTACH_TASK(_mem_tracker)` in `BaseCompaction::execute_compact()` /
`CumulativeCompaction::execute_compact()`, then construct a context while this
shutdown snapshot runs. The constructor registers `this` before `make_shared`
initializes `enable_shared_from_this`'s weak pointer; that later write is
outside `live.lock`, so this `weak_from_this()` read races it (the [C++
shared-pointer
requirements](https://eel.is/c++draft/util.smartptr.shared.const#1) explicitly
make that initialization non-atomic). This introduces undefined behavior in
memleak-check shutdown, and can omit the context from the diagnostic. Ensure
all creators are joined before this call, or synchronize publication and
metadata initia
lization before exposing a context to the registry; cover this overlap with a
concurrency test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
