Signal-safety

Author: Justin Karneges <justin@affinix.com>
Date: March 7th, 2004


Table of Contents


The problem

At some time or another, you might have come across a Qt-based class that cannot be deleted as the result of a signal. Indeed, many classes that are part of Qt have this problem. This is most common with error-handling cases. Consider the following example:

void MyClass::slot_fooError(int err)
{
	delete foo;
	foo = 0;

	QMessageBox::information(0, tr("Error"), tr("Foo error %1").arg(err));
}
Looks sensible enough, yes? However, let us look at Foo's code:

void Foo::doStuff()
{
	++x;
	if(x == 3) {
		emit error(ErrReachedThree);
		x = 0; // reset
	}
}

If you don't see the problem, then you haven't been programming with Qt very long. ;-) Here's what happens: doStuff() invokes slot_fooError(), which deletes foo. This causes the members of foo to be freed. Upon returning from slot_fooError(), doStuff() will now try to set 'x' (a member of foo) to zero. Of course, this is invalid memory now, and the rest is history (as well as your program instance).

Trolltech will tell you that you can't assume a class is deletable as the result of a signal, and you should use deleteLater() to work around this. Fine, we will do this:

void MyClass::slot_fooError(int err)
{
	foo->deleteLater();
	foo = 0;

	QMessageBox::information(0, tr("Error"), tr("Foo error %1").arg(err));
}

Of course, the problem remains. Do you see it? QMessageBox::information() invokes a sub-eventloop, which allows full event processing to take place from this one point in the call stack. This way the messagebox can be dragged around, clicked, etc, and we still have not left slot_fooError(). This is nice, except that now the deleteLater() will go into effect, and so foo will be deleted as soon as this sub-eventloop begins. In other words, because of sub-eventloops, deleteLater() doesn't buy us anything. You can cause similar trouble with qApp->processEvents().

void MyClass::slot_fooError(int err)
{
	foo->deleteLater();
	foo = 0;

	myWidget->show();
	qApp->processEvents(); // make the show() happen NOW
}
Obviously this is not usable. Will reordering it help us?

void MyClass::slot_fooError(int err)
{
	QMessageBox::information(0, tr("Error"), tr("Foo error %1").arg(err));

	foo->deleteLater();
	foo = 0;
}

This, in theory, should work. You could probably debate all day about whether or not the state of MyClass should be reset before or after the QMessageBox. I'd argue that it should happen before, just for cleanliness sake, but I digress. Now, is this reordering of code the master solution? For the purpose of QMessageBox, it's probably fine.

However, consider a situation where your class would rather emit a signal:

void MyClass::slot_fooError(int err)
{
	emit fooFailed();

	foo->deleteLater();
	foo = 0;
}

This simply passes the buck upwards. If the 'owner' of the MyClass instance decides to delete that instance, then it will have to take care that it happens after the fooFailed() signal completes. Otherwise, if MyClass is deleted before that, then the program will crash when foo->deleteLater() is called, as foo would be invalid at that point.


The solution

Rather than pile deleteLater()'s on top of each other, amassing a bunch of QTimers for the purposes of hacky workarounds, I propose that classes always be deletable as the result of a signal. I say that the problem is actually in Foo, so let's fix it:

void Foo::doStuff()
{
	++x;
	if(x == 3) {
		x = 0; // reset before emitting
		emit error(ErrReachedThree);
	}
}

Easy enough, just reorder the code so the resetting happens before the signal. Now we can do this:

void MyClass::slot_fooError(int err)
{
	delete foo;
	foo = 0;

	QMessageBox::information(0, tr("Error"), tr("Foo error %1").arg(err));
}

and this:

void MyClass::slot_fooError(int err)
{
	delete foo;
	foo = 0;

	myWidget->show();
	qApp->processEvents(); // make the show() happen NOW
}

without any worry. Foo will be completely gone at the 'delete foo' line, with no worries of it creeping up later (aside from the fact that doStuff() is harmlessly on the call stack).

The data stack is also a safe place to store data. Consider this variation:

void SomeClass::doIt()
{
	NumList tmp = nums; // nums is a member of SomeClass
	emit listFull(tmp);
	tmp.removeEvens();
	emit listOddsOnly(tmp);
}

The 'owner' of SomeClass can delete the object as the result of either of these signals, because no members are accessed after they emit. The above example may not appear very useful, but keep it in mind, as this stack trick will come into play in the later sections.


Accessing member data after emitting

Now, what if you need to access members after emitting a signal? What if you can't put your signal emit at the end? Consider a socket handling class:

void MySocket::dns_result()
{
	d->address = d->dns->resultAddress();
	emit hostFound();
	d->socket->connect(d->address);
}

Obviously this will crash if MySocket is deleted as a result of emitting hostFound(). How can we fix it? Well, one variation is this:

void MySocket::dns_result()
{
	d->address = d->dns->resultAddress();
	d->socket->connect(d->address);
	emit hostFound();
}

This looks suboptimal, as we should probably emit hostFound() before we start connecting, but the above code could be appropriate, depending on what you're trying to do. It certainly solves the problem, and is clean.

However, there are some dirty ways. Let's look at them. First, via QTimer.

void MySocket::dns_result()
{
	d->address = d->dns->resultAddress();
	QTimer::singleShot(0, this, SLOT(slot_tryConnect()));
	emit hostFound();
}

void MySocket::slot_tryConnect()
{
	d->socket->connect(d->address);
}

This will cause the signal to be emitted prior to performing the followup steps, which might be a more appropriate sequencing for your purposes.

And the other way, via QGuardedPtr:

void MySocket::dns_result()
{
	d->address = d->dns->resultAddress();
	QGuardedPtr<QObject> self = this;
	emit hostFound();
	if(!self)
		return;
	d->socket->connect(d->address);
}

This method ensures that we don't proceed to any code after hostFound() if our class dies during the emit.

Now, both of these are decent solutions and seem to achieve the same thing. However, they do have slightly different behavior. The former uses QTimer, meaning that if the parent invokes a sub-eventloop (such as QMessageBox), then the follow-up code will occur immediately. With the latter, the follow-up code won't happen until the signal is completed, regardless of any sub-eventloops. This means that the eventloops would have to complete (QMessageBoxes would have to be closed), before the code would proceed. This can cause great problems with any code that should be processed even when an exec()'d dialog is open, such as network or process-handling.


QTimer vs QGuardedPtr

It seems we have two choices then. Use QTimer so our follow-up code can run alongside an exec()'d dialog, or use QGuardedPtr and avoid sub-eventloops completely. I vote for the latter, and I'll explain it in one word: sanity.

Using QTimer for signal-safety is tricky. You have to split your function into two parts (making one of them a slot) everytime you add a signal. This can be especially annoying if you are adding a signal to some existing code at a later time.

QGuardedPtr, on the other hand, is easy enough to add to any code. You don't have to consider changing your class code-flow into parts just to add a new signal. In fact, the process is so straightforward that you can make a macro:

#define SAFESIGNAL(a) { QGuardedPtr<QObject> self = this; a; if(!self) return; }

void MySocket::dns_result()
{
	d->address = d->dns->resultAddress();
	SAFESIGNAL(hostFound());
	d->socket->connect(d->address);
}

The drawback of not using QTimer is that your code will not advance if the developer uses QDialog::exec(). My solution? The developer should not use QDialog::exec(). :) Besides, this advancement is just a side-effect of using QTimer. If you were coding without any signal-safety in mind, then you'd just emit your signals wherever you darn please, and you'd still get stuck in an exec().

The conclusion? Code well, and avoid QDialog::exec(). :-)


Working with 'legacy' classes

Now, you might ask: "What if I'm writing code that depends on a class that does not follow these guidelines?" The answer is that it is possible to work around these cases with some nasty hacks. Consider the following example using QSocket, which we assume does not follow our guidelines:

void MyProtocol::MyProtocol()
{
	[...]
	qsock = new QSocket;
	connect(qsock, SIGNAL(bytesWritten(int)), SLOT(qsock_bytesWritten(int)));
	[...]
}

void MyProtocol::~MyProtocol()
{
	[ ... ]
	delete qsock;
	[ ... ]
}

void MyProtocol::qsock_bytesWritten(int x)
{
	bool packetWritten = [ ... packet-size processing involving 'x' ... ];
	if(packetWritten)
		emit packetSent();
}

This looks harmless at first. After all, you're emitting the signal at the end of that function, with no further access to members, right? Yes, this is true, but you don't know what is going on within QSocket. It might try to access its own members after this call, which would be unsafe if we are deleted during our signal.

Fine, we switch to deleteLater(),

void MyProtocol::~MyProtocol()
{
	[ ... ]
	qsock->deleteLater();
	[ ... ]
}

Of course, this doesn't solve the QMessageBox problem. To completely fix this for good, we must use the stack:

void MyProtocol::MyProtocol()
{
	[...]
	inQSockSignal = false;
	qsock = new QSocket;
	connect(qsock, SIGNAL(bytesWritten(int)), SLOT(qsock_bytesWritten(int)));
	[...]
}

void MyProtocol::~MyProtocol()
{
	[ ... ]
	// don't delete the qsocket if we're in one of its signals
	if(!inQSockSignal)
		qsock->deleteLater();
	[ ... ]
}

void MyProtocol::qsock_bytesWritten(int x)
{
	bool packetWritten = [ ... packet-size processing involving 'x' ... ];
	if(packetWritten) {
		inQSockSignal = true;
		QSocket *tmp = qsock;
		QGuardedPtr<QObject> self = this;
		emit packetSent();
		if(!self) {
			// if we got deleted here, then the qsocket is still alive.  delete it now
			tmp->deleteLater();
			return;
		}
		inQSockSignal = false;
	}
}

Tricky, eh? ;-)


cutestuff's SafeDelete

In order to make the previous concept easier to follow, I've created the SafeDelete class, which can be found in my cutestuff/util collection. Just two files are needed (safedelete.h and safedelete.cpp). It works like this:

void MyProtocol::MyProtocol()
{
	[...]
	sd = new SafeDelete; // make one of these
	qsock = new QSocket;
	connect(qsock, SIGNAL(bytesWritten(int)), SLOT(qsock_bytesWritten(int)));
	[...]
}

void MyProtocol::~MyProtocol()
{
	[ ... ]
	sd->deleteLater(d->qsock); // go through SafeDelete instead of direct
	[ ... ]
	delete sd;
}

void MyProtocol::qsock_bytesWritten(int x)
{
	SafeDeleteLock s(sd); // lock the function

	bool packetWritten = [ ... packet-size processing involving 'x' ... ];
	if(packetWritten)
		emit packetSent();
}

Basically, SafeDelete will normally call deleteLater() on the passed object, unless there is an active SafeDeleteLock on it. If there is a SafeDeleteLock, then SafeDelete simply queues up the delete requests. If the SafeDeleteLock is destroyed, then the delete requests are processed. So far so good. Now, if the SafeDelete is destroyed, then the destructor moves the delete queue into the SafeDeleteLock, which is on the stack! This way, MyProtocol can safely be deleted. When the SafeDeleteLock is destructed, the delete requests are processed.

Note: In the above example, SafeDeleteLock is created on the stack, so that it is destroyed automatically when the function ends. This is similar in nature to how QMutexLocker works.

Happy coding!