From 2a9c63766d541ab642b4d304983c48dd07bf7ac9 Mon Sep 17 00:00:00 2001 From: Vlad Khorsun Date: Wed, 17 Jul 2024 19:17:58 +0300 Subject: [PATCH 1/3] Let use same name for event_t::event_pid on all platforms. Add asserts to check PID for event waiting routines. --- src/common/isc_s_proto.h | 2 +- src/common/isc_sync.cpp | 10 +++++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/common/isc_s_proto.h b/src/common/isc_s_proto.h index 582aa09d5c6..6d8ab54cefb 100644 --- a/src/common/isc_s_proto.h +++ b/src/common/isc_s_proto.h @@ -73,7 +73,7 @@ struct mtx struct event_t { SLONG event_count; - int pid; + int event_pid; pthread_mutex_t event_mutex[1]; pthread_cond_t event_cond[1]; }; diff --git a/src/common/isc_sync.cpp b/src/common/isc_sync.cpp index 7b5cc6df783..9a3d98bce9c 100755 --- a/src/common/isc_sync.cpp +++ b/src/common/isc_sync.cpp @@ -611,7 +611,7 @@ int SharedMemoryBase::eventInit(event_t* event) #else // pthread-based event event->event_count = 0; - event->pid = getpid(); + event->event_pid = getpid(); // Prepare an Inter-Process event block pthread_mutexattr_t mattr; @@ -662,7 +662,7 @@ void SharedMemoryBase::eventFini(event_t* event) #else // pthread-based event - if (event->pid == getpid()) + if (event->event_pid == getpid()) { LOG_PTHREAD_ERROR(pthread_mutex_destroy(event->event_mutex)); LOG_PTHREAD_ERROR(pthread_cond_destroy(event->event_cond)); @@ -691,6 +691,8 @@ SLONG SharedMemoryBase::eventClear(event_t* event) * **************************************/ + fb_assert(event->event_pid == getpid()); + #if defined(WIN_NT) ResetEvent(event->event_handle); @@ -722,6 +724,8 @@ int SharedMemoryBase::eventWait(event_t* event, const SLONG value, const SLONG m * **************************************/ + fb_assert(event->event_pid == getpid()); + // If we're not blocked, the rest is a gross waste of time if (!event_blocked(event, value)) { @@ -831,7 +835,7 @@ int SharedMemoryBase::eventPost(event_t* event) ++event->event_count; if (event->event_pid != process_id) - return ISC_kill(event->event_pid, event->event_id, event->event_handle); + return ISC_kill(event->event_pid, event->event_id, event->event_handle) == 0 ? FB_SUCCESS : FB_FAILURE; return SetEvent(event->event_handle) ? FB_SUCCESS : FB_FAILURE; From 79dfe14465c82bbafcf0f0f540ff0efc7435fe68 Mon Sep 17 00:00:00 2001 From: Vlad Khorsun Date: Wed, 17 Jul 2024 19:23:18 +0300 Subject: [PATCH 2/3] Avoid deadlock between shutdownAttachments/purge_attachment/release_attachment/.../~ProfilerListener() and ProfilerListener::watcherThread/EngineContextHolder --- src/jrd/Attachment.cpp | 13 +++++++++++-- src/jrd/Attachment.h | 2 +- src/jrd/ProfilerManager.cpp | 9 ++++++--- src/jrd/ProfilerManager.h | 5 +++++ src/jrd/jrd.cpp | 4 ++-- 5 files changed, 25 insertions(+), 8 deletions(-) diff --git a/src/jrd/Attachment.cpp b/src/jrd/Attachment.cpp index 6143ac21276..0d3f079eb50 100644 --- a/src/jrd/Attachment.cpp +++ b/src/jrd/Attachment.cpp @@ -1207,7 +1207,16 @@ bool Attachment::isProfilerActive() return att_profiler_manager && att_profiler_manager->isActive(); } -void Attachment::releaseProfilerManager() +void Attachment::releaseProfilerManager(thread_db* tdbb) { - att_profiler_manager.reset(); + if (!att_profiler_manager) + return; + + if (att_profiler_manager->haveListener()) + { + EngineCheckout cout(tdbb, FB_FUNCTION); + att_profiler_manager.reset(); + } + else + att_profiler_manager.reset(); } diff --git a/src/jrd/Attachment.h b/src/jrd/Attachment.h index 30b67b3e79d..9128297990f 100644 --- a/src/jrd/Attachment.h +++ b/src/jrd/Attachment.h @@ -843,7 +843,7 @@ class Attachment : public pool_alloc ProfilerManager* getProfilerManager(thread_db* tdbb); ProfilerManager* getActiveProfilerManagerForNonInternalStatement(thread_db* tdbb); bool isProfilerActive(); - void releaseProfilerManager(); + void releaseProfilerManager(thread_db* tdbb); JProvider* getProvider() { diff --git a/src/jrd/ProfilerManager.cpp b/src/jrd/ProfilerManager.cpp index 00d59c5f66d..6483ed49e17 100644 --- a/src/jrd/ProfilerManager.cpp +++ b/src/jrd/ProfilerManager.cpp @@ -385,10 +385,13 @@ int ProfilerManager::blockingAst(void* astObject) const auto dbb = attachment->att_database; AsyncContextHolder tdbb(dbb, FB_FUNCTION, attachment->att_profiler_listener_lock); - const auto profilerManager = attachment->getProfilerManager(tdbb); + if (!(attachment->att_flags & ATT_shutdown)) + { + const auto profilerManager = attachment->getProfilerManager(tdbb); - if (!profilerManager->listener) - profilerManager->listener = FB_NEW_POOL(*attachment->att_pool) ProfilerListener(tdbb); + if (!profilerManager->listener) + profilerManager->listener = FB_NEW_POOL(*attachment->att_pool) ProfilerListener(tdbb); + } LCK_release(tdbb, attachment->att_profiler_listener_lock); } diff --git a/src/jrd/ProfilerManager.h b/src/jrd/ProfilerManager.h index 86695d10a8a..0b340f37a16 100644 --- a/src/jrd/ProfilerManager.h +++ b/src/jrd/ProfilerManager.h @@ -273,6 +273,11 @@ class ProfilerManager final : public Firebird::PerformanceStopWatch return currentSession && !paused; } + bool haveListener() const + { + return listener.hasData(); + } + static void checkFlushInterval(SLONG interval) { if (interval < 0) diff --git a/src/jrd/jrd.cpp b/src/jrd/jrd.cpp index edd68af4bcc..744db2def05 100644 --- a/src/jrd/jrd.cpp +++ b/src/jrd/jrd.cpp @@ -7670,8 +7670,6 @@ void release_attachment(thread_db* tdbb, Jrd::Attachment* attachment, XThreadEns if (!attachment) return; - attachment->releaseProfilerManager(); - attachment->att_replicator = nullptr; if (attachment->att_dsql_instance) @@ -8408,6 +8406,8 @@ static void purge_attachment(thread_db* tdbb, StableAttachmentPart* sAtt, unsign } } + attachment->releaseProfilerManager(tdbb); + // stop crypt thread using this attachment dbb->dbb_crypto_manager->stopThreadUsing(tdbb, attachment); From ff0f3cf93ffaec45004c77daa16033c017d5da26 Mon Sep 17 00:00:00 2001 From: Vlad Khorsun Date: Thu, 18 Jul 2024 11:25:16 +0300 Subject: [PATCH 3/3] Use correct way to initialize shared events. Fixed few race conditions and hangs. Unlink file used for shared memory when it is gets unused. --- src/jrd/ProfilerManager.cpp | 221 ++++++++++++++++++++++++++++-------- 1 file changed, 171 insertions(+), 50 deletions(-) diff --git a/src/jrd/ProfilerManager.cpp b/src/jrd/ProfilerManager.cpp index 6483ed49e17..1eed24f0c1e 100644 --- a/src/jrd/ProfilerManager.cpp +++ b/src/jrd/ProfilerManager.cpp @@ -34,6 +34,8 @@ #include "../jrd/pag_proto.h" #include "../jrd/tra_proto.h" +#include + using namespace Jrd; using namespace Firebird; @@ -50,10 +52,14 @@ namespace { NOP = 0, + SERVER_STARTED, + SERVER_EXITED, + RESPONSE, EXCEPTION, - CANCEL_SESSION, + FIRST_CLIENT_OP, + CANCEL_SESSION = FIRST_CLIENT_OP, DISCARD, FINISH_SESSION, FLUSH, @@ -89,15 +95,16 @@ namespace event_t serverEvent; event_t clientEvent; USHORT bufferSize; - Tag tag; + std::atomic tag; char userName[USERNAME_LENGTH + 1]; // \0 if has PROFILE_ANY_ATTACHMENT alignas(FB_ALIGNMENT) UCHAR buffer[4096]; }; - static const USHORT VERSION = 1; + static const USHORT VERSION = 2; public: - ProfilerIpc(thread_db* tdbb, MemoryPool& pool, AttNumber aAttachmentId); + ProfilerIpc(thread_db* tdbb, MemoryPool& pool, AttNumber aAttachmentId, bool server = false); + ~ProfilerIpc(); ProfilerIpc(const ProfilerIpc&) = delete; ProfilerIpc& operator=(const ProfilerIpc&) = delete; @@ -138,10 +145,12 @@ namespace private: void internalSendAndReceive(thread_db* tdbb, Tag tag, const void* in, unsigned inSize, void* out, unsigned outSize); + void initClient(); public: AutoPtr> sharedMemory; AttNumber attachmentId; + const bool isServer; }; } // anonymous namespace @@ -748,8 +757,9 @@ ProfilerManager::Statement* ProfilerManager::getStatement(Request* request) //-------------------------------------- -ProfilerIpc::ProfilerIpc(thread_db* tdbb, MemoryPool& pool, AttNumber aAttachmentId) - : attachmentId(aAttachmentId) +ProfilerIpc::ProfilerIpc(thread_db* tdbb, MemoryPool& pool, AttNumber aAttachmentId, bool server) + : attachmentId(aAttachmentId), + isServer(server) { const auto database = tdbb->getDatabase(); @@ -767,7 +777,33 @@ ProfilerIpc::ProfilerIpc(thread_db* tdbb, MemoryPool& pool, AttNumber aAttachmen throw; } - checkHeader(sharedMemory->getHeader()); + const auto header = sharedMemory->getHeader(); + checkHeader(header); + + if (isServer) + { + Guard guard(this); + + if (sharedMemory->eventInit(&header->serverEvent) != FB_SUCCESS) + (Arg::Gds(isc_random) << "ProfilerIpc eventInit(serverEvent) failed").raise(); + } +} + +ProfilerIpc::~ProfilerIpc() +{ + Guard guard(this); + + const auto header = sharedMemory->getHeader(); + + event_t* evnt = this->isServer ? &header->serverEvent : &header->clientEvent; + if (evnt->event_pid) + { + sharedMemory->eventFini(evnt); + evnt->event_pid = 0; + } + + if (header->serverEvent.event_pid == 0 && header->clientEvent.event_pid == 0) + sharedMemory->removeMapFile(); } bool ProfilerIpc::initialize(SharedMemoryBase* sm, bool init) @@ -778,15 +814,6 @@ bool ProfilerIpc::initialize(SharedMemoryBase* sm, bool init) // Initialize the shared data header. initHeader(header); - - if (sm->eventInit(&header->serverEvent) != FB_SUCCESS) - (Arg::Gds(isc_random) << "ProfilerIpc eventInit(serverEvent) failed").raise(); - - if (sm->eventInit(&header->clientEvent) != FB_SUCCESS) - { - sm->eventFini(&header->serverEvent); - (Arg::Gds(isc_random) << "ProfilerIpc eventInit(clientEvent) failed").raise(); - } } return true; @@ -828,7 +855,34 @@ void ProfilerIpc::internalSendAndReceive(thread_db* tdbb, Tag tag, const auto header = sharedMemory->getHeader(); - header->tag = tag; + initClient(); + + Cleanup finiClient([&] { + if (header->clientEvent.event_pid) + { + sharedMemory->eventFini(&header->clientEvent); + header->clientEvent.event_pid = 0; + } + }); + + const SLONG value = sharedMemory->eventClear(&header->clientEvent); + + const Tag oldTag = header->tag.exchange(tag); + switch (oldTag) + { + case Tag::NOP: + header->tag = oldTag; + (Arg::Gds(isc_random) << "Remote attachment failed to start listener thread").raise(); + break; + + case Tag::SERVER_EXITED: + header->tag = oldTag; + (Arg::Gds(isc_random) << "Cannot start remote profile session - attachment exited").raise(); + break; + + default: + break; + }; if (attachment->locksmith(tdbb, PROFILE_ANY_ATTACHMENT)) header->userName[0] = '\0'; @@ -840,25 +894,86 @@ void ProfilerIpc::internalSendAndReceive(thread_db* tdbb, Tag tag, fb_assert(inSize <= sizeof(header->buffer)); memcpy(header->buffer, in, inSize); - const SLONG value = sharedMemory->eventClear(&header->clientEvent); - - sharedMemory->eventPost(&header->serverEvent); + if (sharedMemory->eventPost(&header->serverEvent) != FB_SUCCESS) + (Arg::Gds(isc_random) << "Cannot start remote profile session - attachment exited").raise(); { - EngineCheckout cout(tdbb, FB_FUNCTION); - sharedMemory->eventWait(&header->clientEvent, value, 0); + const SLONG TIMEOUT = 500 * 1000; // 0.5 sec + + const int serverPID = header->serverEvent.event_pid; + while (true) + { + { + EngineCheckout cout(tdbb, FB_FUNCTION); + if (sharedMemory->eventWait(&header->clientEvent, value, TIMEOUT) == FB_SUCCESS) + break; + + if (serverPID != getpid() && !ISC_check_process_existence(serverPID)) + { + // Server process was died or exited + fb_assert((header->tag == tag) || header->tag == Tag::SERVER_EXITED); + + if (header->tag == tag) + { + header->tag = Tag::SERVER_EXITED; + if (header->serverEvent.event_pid) + { + sharedMemory->eventFini(&header->serverEvent); + header->serverEvent.event_pid = 0; + } + } + break; + } + } + JRD_reschedule(tdbb, true); + } } - if (header->tag == Tag::RESPONSE) + switch (header->tag) { + case Tag::SERVER_EXITED: + (Arg::Gds(isc_random) << "Cannot start remote profile session - attachment exited").raise(); + break; + + case Tag::RESPONSE: fb_assert(outSize == header->bufferSize); memcpy(out, header->buffer, header->bufferSize); + break; + + case Tag::EXCEPTION: + (Arg::Gds(isc_random) << (char*) header->buffer).raise(); + break; + + default: + fb_assert(false); } - else +} + +void ProfilerIpc::initClient() +{ + // Shared memory mutex must be locked by caller + + fb_assert(isServer == false); + + const auto header = sharedMemory->getHeader(); + + // Here should not be event created by another alive client + + if (header->clientEvent.event_pid) { - fb_assert(header->tag == Tag::EXCEPTION); - (Arg::Gds(isc_random) << (char*) header->buffer).raise(); + fb_assert(header->clientEvent.event_pid != getpid()); + + if (header->clientEvent.event_pid != getpid()) + { + if (ISC_check_process_existence(header->clientEvent.event_pid)) + (Arg::Gds(isc_random) << "ProfilerIpc eventInit(clientEvent) failed").raise(); + } + + sharedMemory->eventFini(&header->clientEvent); } + + if (sharedMemory->eventInit(&header->clientEvent) != FB_SUCCESS) + (Arg::Gds(isc_random) << "ProfilerIpc eventInit(clientEvent) failed").raise(); } @@ -871,9 +986,10 @@ ProfilerListener::ProfilerListener(thread_db* tdbb) { auto& pool = *attachment->att_pool; - ipc = FB_NEW_POOL(pool) ProfilerIpc(tdbb, pool, attachment->att_attachment_id); + ipc = FB_NEW_POOL(pool) ProfilerIpc(tdbb, pool, attachment->att_attachment_id, true); cleanupSync.run(this); + startupSemaphore.enter(); } ProfilerListener::~ProfilerListener() @@ -881,19 +997,14 @@ ProfilerListener::~ProfilerListener() exiting = true; // Terminate the watcher thread. - startupSemaphore.tryEnter(5); - - ProfilerIpc::Guard guard(ipc); - - auto& sharedMemory = ipc->sharedMemory; - - sharedMemory->eventPost(&sharedMemory->getHeader()->serverEvent); - cleanupSync.waitForCompletion(); - const auto header = sharedMemory->getHeader(); + if (ipc) + { + auto& sharedMemory = ipc->sharedMemory; + sharedMemory->eventPost(&sharedMemory->getHeader()->serverEvent); - sharedMemory->eventFini(&header->serverEvent); - sharedMemory->eventFini(&header->clientEvent); + cleanupSync.waitForCompletion(); + } } void ProfilerListener::exceptionHandler(const Exception& ex, ThreadFinishSync::ThreadRoutine*) @@ -904,23 +1015,32 @@ void ProfilerListener::exceptionHandler(const Exception& ex, ThreadFinishSyncsharedMemory; + const auto header = sharedMemory->getHeader(); + + fb_assert(header->tag == ProfilerIpc::Tag::NOP); + header->tag = ProfilerIpc::Tag::SERVER_STARTED; try { while (!exiting) { - auto& sharedMemory = ipc->sharedMemory; - const auto header = sharedMemory->getHeader(); - const SLONG value = sharedMemory->eventClear(&header->serverEvent); - if (header->tag != ProfilerIpc::Tag::NOP) + if (startup) + { + startup = false; + startupSemaphore.release(); + } + else { - FbLocalStatus statusVector; - EngineContextHolder tdbb(&statusVector, attachment->getInterface(), FB_FUNCTION); + fb_assert(header->tag >= ProfilerIpc::Tag::FIRST_CLIENT_OP); try { + FbLocalStatus statusVector; + EngineContextHolder tdbb(&statusVector, attachment->getInterface(), FB_FUNCTION); + processCommand(tdbb); header->tag = ProfilerIpc::Tag::RESPONSE; } @@ -950,12 +1070,6 @@ void ProfilerListener::watcherThread() sharedMemory->eventPost(&header->clientEvent); } - if (startup) - { - startup = false; - startupSemaphore.release(); - } - if (exiting) break; @@ -967,6 +1081,13 @@ void ProfilerListener::watcherThread() iscLogException("Error in profiler watcher thread\n", ex); } + const ProfilerIpc::Tag oldTag = header->tag.exchange(ProfilerIpc::Tag::SERVER_EXITED); + if (oldTag >= ProfilerIpc::Tag::FIRST_CLIENT_OP) + { + fb_assert(header->clientEvent.event_pid); + sharedMemory->eventPost(&header->clientEvent); + } + try { if (startup)