Fixed bug #8151 : Deadlock happens when run 'List Trace Sessions' service and there are many active trace sessions

src/jrd/trace/TraceConfigStorage.cpp

@@ -90,8 +90,7 @@ ConfigStorage::ConfigStorage()
 	  m_filename(getPool()),
 	  m_recursive(0),
 	  m_mutexTID(0),
-	  m_dirty(false),
-	  m_nextIdx(0)
+	  m_dirty(false)
 {
 #ifdef WIN_NT
 	DWORD sesID = 0;
@@ -744,19 +743,14 @@ bool ConfigStorage::getSession(Firebird::TraceSession& session, GET_FLAGS getFla
 	return readSession(slot, session, getFlag);
 }
 
-void ConfigStorage::restart()
-{
-	m_nextIdx = 0;
-}
-
-bool ConfigStorage::getNextSession(TraceSession& session, GET_FLAGS getFlag)
+bool ConfigStorage::getNextSession(TraceSession& session, GET_FLAGS getFlag, ULONG& nextIdx)
 {
 	TraceCSHeader* header = m_sharedMemory->getHeader();
 
-	while (m_nextIdx < header->slots_cnt)
+	while (nextIdx < header->slots_cnt)
 	{
-		TraceCSHeader::Slot* slot = header->slots + m_nextIdx;
-		m_nextIdx++;
+		TraceCSHeader::Slot* slot = header->slots + nextIdx;
+		nextIdx++;
 
 		if (slot->used)
 			return readSession(slot, session, getFlag);
@@ -895,6 +889,31 @@ void ConfigStorage::updateFlags(TraceSession& session)
 	slot->ses_flags = session.ses_flags;
 }
 
+bool ConfigStorage::Accessor::getNext(TraceSession& session, GET_FLAGS getFlag)
+{
+	if (m_guard)
+		return m_storage->getNextSession(session, getFlag, m_nextIdx);
+
+	StorageGuard guard(m_storage);
+
+	// Restore position, if required: find index of slot with session ID greater than m_sesId.
+	if (m_change_number != m_storage->getChangeNumber())
+	{
+		if (m_storage->findSession(m_sesId, m_nextIdx))
+			m_nextIdx++;
+
+		m_change_number = m_storage->getChangeNumber();
+	}
+
+	if (m_storage->getNextSession(session, getFlag, m_nextIdx))
+	{
+		m_sesId = session.ses_id;
+		return true;
+	}
+
+	return false;
+}
+
 void ConfigStorage::Writer::write(ITEM tag, ULONG len, const void* data)
 {
 	if (m_mem + 1 > m_end)

src/jrd/trace/TraceConfigStorage.h

@@ -49,6 +49,8 @@ namespace Jrd {
   Slot is reused with best-fit algorithm.
 */
 
+class StorageGuard;
+
 struct TraceCSHeader : public Firebird::MemoryHeader
 {
 	static const USHORT TRACE_STORAGE_VERSION = 2;
@@ -97,9 +99,6 @@ public:
 	// get session by sesion id
 	bool getSession(Firebird::TraceSession& session, GET_FLAGS getFlag);
 
-	void restart();
-	bool getNextSession(Firebird::TraceSession& session, GET_FLAGS getFlag);
-
 	ULONG getChangeNumber() const
 	{ return m_sharedMemory && m_sharedMemory->getHeader() ? m_sharedMemory->getHeader()->change_number : 0; }
 
@@ -110,6 +109,35 @@ public:
 
 	Firebird::Mutex m_localMutex;
 
+	class Accessor
+	{
+	public:
+		// Use when storage is not locked by caller
+		explicit Accessor(ConfigStorage* storage) :
+			m_storage(storage),
+			m_guard(nullptr)
+		{}
+
+		// Use when storage is locked by caller
+		explicit Accessor(StorageGuard* guard);
+
+		void restart()
+		{
+			m_change_number = 0;
+			m_sesId = 0;
+			m_nextIdx = 0;
+		}
+
+		bool getNext(Firebird::TraceSession& session, GET_FLAGS getFlag);
+
+	private:
+		ConfigStorage* const m_storage;
+		StorageGuard* const m_guard;
+		ULONG m_change_number = 0;
+		ULONG m_sesId = 0;					// last seen session ID
+		ULONG m_nextIdx = 0;				// slot index next after last seen one
+	};
+
 private:
 	void mutexBug(int osErrorCode, const char* text) override;
 	bool initialize(Firebird::SharedMemoryBase*, bool) override;
@@ -181,6 +209,10 @@ private:
 	bool findSession(ULONG sesId, ULONG& idx);
 	bool readSession(TraceCSHeader::Slot* slot, Firebird::TraceSession& session, GET_FLAGS getFlag);
 
+	// Search for used slot starting from nextIdx and increments nextIdx to point to the next slot
+	// returns false, if used slot was not found
+	bool getNextSession(Firebird::TraceSession& session, GET_FLAGS getFlag, ULONG& nextIdx);
+
 	class Reader
 	{
 	public:
@@ -217,7 +249,6 @@ private:
 	int m_recursive;
 	ThreadId m_mutexTID;
 	bool m_dirty;
-	ULONG m_nextIdx;	// getNextSession() iterator index
 };
 
 
@@ -265,10 +296,22 @@ public:
 	{
 		m_storage->release();
 	}
+
+	ConfigStorage* getStorage()
+	{
+		return m_storage;
+	}
+
 private:
 	ConfigStorage* m_storage;
 };
 
+
+inline ConfigStorage::Accessor::Accessor(StorageGuard* guard) :
+	m_storage(guard->getStorage()),
+	m_guard(guard)
+{}
+
 }
 
 #endif

src/jrd/trace/TraceManager.cpp

@@ -193,10 +193,10 @@ void TraceManager::update_sessions()
 		const bool noNewSessions = attachment && (attachment->att_purge_tid);
 
 		StorageGuard guard(storage);
-		storage->restart();
+		ConfigStorage::Accessor acc(&guard);
 
 		TraceSession session(pool);
-		while (storage->getNextSession(session, ConfigStorage::FLAGS))
+		while (acc.getNext(session, ConfigStorage::FLAGS))
 		{
 			if ((session.ses_flags & trs_active) && !(session.ses_flags & trs_log_full))
 			{

src/jrd/trace/TraceService.cpp

@@ -228,13 +228,15 @@ void TraceSvcJrd::listSessions()
 {
 	m_svc.started();
 
-	ConfigStorage* storage = TraceManager::getStorage();
-	StorageGuard guard(storage);
+	// Writing into service when storage is locked could lead to deadlock,
+	// therefore don't use StorageGuard here.
 
-	storage->restart();
+	ConfigStorage* storage = TraceManager::getStorage();
+	ConfigStorage::Accessor acc(storage);
 
 	TraceSession session(*getDefaultMemoryPool());
-	while (storage->getNextSession(session, ConfigStorage::ALL))
+
+	while (acc.getNext(session, ConfigStorage::ALL))
 	{
 		if (checkPrivileges(session))
 		{