Valgrind / Multi-Threaded Question

[pieant86]

Hi everyone,

Just an update on a memory leak issue I’ve been tracking — I believe I’ve found the cause and successfully fixed it.

Summary of the Issue
The leak occurs during cross-thread signal emission, specifically when the signal and slot are connected with a QueuedConnection. The key problem is that a BentoAbstract pointer is cloned and stored, but never deleted, leading to a leak. Here's the relevant call flow:

Code: Select all

SUMMARY OF CALL FLOW
Step    Function                Action
1       signal()                Emits signal
2       CsSignal::activate()    Clones signal & slot Bento, wraps data, creates PendingSlot
3       QObject::queueSlot()    Releases slot_Bento raw pointer into CSMetaCallEvent
4       CSMetaCallEvent         Stores pointer but doesn’t manage it
5       ~CSMetaCallEvent()      Deletes data, but not the m_bento
= Leak occurs, memory allocated in clone() never gets freed

This behavior triggers a definite memory leak detected by Valgrind (32 bytes per signal).

There's a missing "delete" on "CSMetaCallEvent" destructor into the "https://github.com/copperspice/copperspice/blob/master/src/core/kernel/csmeta_callevent.cpp" file.

The fix is simple: ensure that CSMetaCallEvent properly deletes the m_bento member in its destructor. Here’s the patched version of csmeta_callevent.cpp:

Code: Select all

/***********************************************************************
*
* Copyright (c) 2012-2024 Barbara Geller
* Copyright (c) 2012-2024 Ansel Sermersheim
*
* Copyright (c) 2015 The Qt Company Ltd.
* Copyright (c) 2012-2016 Digia Plc and/or its subsidiary(-ies).
* Copyright (c) 2008-2012 Nokia Corporation and/or its subsidiary(-ies).
*
* This file is part of CopperSpice.
*
* CopperSpice is free software. You can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public License
* version 2.1 as published by the Free Software Foundation.
*
* CopperSpice is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
*
* https://www.gnu.org/licenses/
*
***********************************************************************/

// do not move the order of these two includes
#include <qobject.h>
#include <csmeta_callevent.h>

// internal class
CSMetaCallEvent::CSMetaCallEvent(const CsSignal::Internal::BentoAbstract *bento,
      const CsSignal::Internal::TeaCupAbstract *dataPack,
      const QObject *sender, int signal_index, QSemaphore *semaphore)
   : QEvent(MetaCall), m_bento(bento), m_dataPack(dataPack), m_sender(sender),
     m_semaphore(semaphore), m_signal_index(signal_index)
{
}

CSMetaCallEvent::~CSMetaCallEvent()
{
	delete m_dataPack;
	
	///////////////////////////////////////////////////////////////////////////////////
	#warning "@src\core\kernel\csmeta_callevent.cpp m_bento leak fix!!!"
	//SUMMARY OF CALL FLOW
	//Step    Function                Action
	//1       signal()                Emits signal
	//2       CsSignal::activate()    Clones signal & slot Bento, wraps data, creates PendingSlot
	//3       QObject::queueSlot()    Releases slot_Bento raw pointer into CSMetaCallEvent
	//4       CSMetaCallEvent         Stores pointer but doesn’t manage it
	//5       ~CSMetaCallEvent()      Deletes data, but not the m_bento
	//= Leak occurs Memory allocated in clone() never gets freed
	//if(m_bento){ qWarning("CSMetaCallEvent::~CSMetaCallEvent() Forcing m_bento deallocation"); }
	delete m_bento;     // PAS: MEMORY LEAK FIX!
	///////////////////////////////////////////////////////////////////////////////////

	if (m_semaphore) {
	  m_semaphore->release();
	}
}

void CSMetaCallEvent::placeMetaCall(QObject *object)
{
   m_bento->invoke(object, m_dataPack);
}

const QObject *CSMetaCallEvent::sender() const
{
   return m_sender;
}

int CSMetaCallEvent::signal_index() const
{
   return m_signal_index;
}

Test Results
After rebuilding libCsCore with this patch, I reran my test scenario, and Valgrind no longer reports any memory leaks for this case.

Request for Review
Could someone from the core team please review this and, if confirmed, consider applying it to the official repository?

Thanks in advance, and thanks for all the work on CopperSpice — it’s been a great framework to work with!

[barbara]

Thank you for reporting this memory issue. Our CS team has been able to confirm this problem with additional testing.

Your solution appears to prevent the memory leak. However, we looked at the code a bit more and believe this can be resolved more efficiently by using unique pointers to manage the memory.

We have this on our current schedule. If you would like to continue working on this, we are happy to banter our ideas and then have you submit a pull request.

Barbara