Delta XMPP Project

Here is the source code for Qt 4.2.2’s QUdpSocket::writeDatagram:

qint64 QUdpSocket::writeDatagram(const char *data, qint64 size,
    const QHostAddress &address,
    quint16 port)
{
    Q_D(QUdpSocket);
#if defined QUDPSOCKET_DEBUG
    qDebug("QUdpSocket::writeDatagram(%p, %llu, "%s", %i)", data, size,
           address.toString().toLatin1().constData(), port);
#endif
    QT_ENSURE_INITIALIZED(-1);
    qint64 sent = d->socketEngine->writeDatagram(data, size, address, port);
    if (sent >= 0) {
        emit bytesWritten(sent);
    } else {
        d->socketError = d->socketEngine->error();
        setErrorString(d->socketEngine->errorString());
        emit error(d->socketError);
    }
    return sent;
}

If this code looks sane to you, then you’ve already forgotten your lesson on Delayed Signals. Here we have a function that should clearly be using delayed signals, since the signals are being used “as a means of returning or indicating a result of a requested operation” (I need a shorter term for this :)). The linked article already goes over why delayed signals should be used with request-based operations, but I’ll go over the concept again below in an example with QUdpSocket.

Suppose you wanted to put “Hello World!” inside of a UDP packet. Suppose you wanted to send such a packet to some computer on port 2000. Suppose you wanted to send a lot of these. I suppose you’d end up with something like PacketSender:

class PacketSender
{
        Q_OBJECT
public:
        PacketSender()
        {
                sock = new QUdpSocket(this);
                connect(sock, SIGNAL(bytesWritten(qint64)),
                        SLOT(sock_bytesWritten(qint64)));
                connect(sock, SIGNAL(error(QAbstractSocket::SocketError)),
                        SLOT(sock_error(QAbstractSocket::SocketError)));
        }

        void start(const QList<QHostAddress> &addrs)
        {
                QByteArray packet = "Hello World!";
                total = addrs.count();
                foreach(QHostAddress addr, addrs)
                {
                        sock->writeDatagram(packet.data(), packet.size(),
                                addr, 2000);
                }
        }

signals:
        void finished();
        void error();

private slots:
        void sock_bytesWritten(qint64)
        {
                --total;
                if(total == 0)
                        emit finished();
        }

        void sock_error(QAbstractSocket::SocketError)
        {
                delete sock;
                emit error();
        }

private:
        QUdpSocket *sock;
        int total;
};

With PacketSender, you instantiate the object, connect your signals, and call start() with a list of addresses to send the packets to. PacketSender will emit finished() when all packets have been sent successfully, or error() if it runs into any problems while sending. Either signal shall indicate the end of the operation. Example use:

PacketSender *sender = new PacketSender;
connect(sender, SIGNAL(finished()), ...);
connect(sender, SIGNAL(error()), ...);
QList<QHostAddress> addrs;
addrs += "192.168.0.1";
addrs += "192.168.0.2";
sender->start(addrs);

The code for PacketSender looks good. In fact, this code is probably perfect for its intent. However, there are some problems here, and it is because we inherit these problems from QUdpSocket. This is quite common among my Qt guidelines. Misbehaving classes must be compensated for if you use them inside of your own classes, otherwise users will have to compensate for *your* class. Ultimately, either the developer or the user of a class will have to make things work. My philosophy is to put the greater burden on the developer of the class, as a class is developed once but used many times.

The first problem with PacketSender is that it does not obey the delayed signals guideline. It looks like it should, but QUdpSocket’s behavior causes PacketSender to break the rules. For example, finished() might be emitted before start() returns.

One simple solution would be to delay the call to writeDatagram, by reworking it like this:

...
public:
...
        void start(const QList<QHostAddress> &addrs)
        {
                addrs = _addrs;
                QTimer::singleShot(0, this, SLOT(real_start()));
        }
...

public slots:
...
        void real_start()
        {
                QByteArray packet = "Hello World!";
                total = addrs.count();
                foreach(QHostAddress addr, addrs)
                {
                        sock->writeDatagram(packet.data(), packet.size(),
                                addr, 2000);
                }
        }
...
private:
...
        QList<QHostAddress> addrs;
...

The second problem with PacketSender is that it does not obey the signal safety guideline, and this stems from its assumption that QUdpSocket is signal-safe. This assumption should never be made unless the class or a specific function/signal is known or labeled to be signal-safe (no Qt classes are). Funny enough, by peeking into the code, we can see that writeDatagram()’s code is actually signal-safe. Also, judging by the fact that the function is guaranteed to emit either bytesWritten() or error(), we can probably assume that these signals are never emitted as a result of any other operation.

However, PacketSender::start() is not signal-safe due to QUdpSocket not practicing delayed signals. If PacketSender is deleted during the finished() or error() signal, start() will still be on the call stack, and the next iteration of the foreach loop will cause a crash. By employing the delayed signals fix, PacketSender should in theory be signal-safe, because then start() won’t be on the call stack.

That said, since QUdpSocket is not documented as being signal-safe, I don’t think it is something we should rely on. Here’s the fix:

...
public:
        ~PacketSender()
        {
                sock->disconnect(this);
                sock->setParent(0); // prevent auto-delete
                sock->deleteLater();
        }
...

Yay, PacketSender is now signal-safe and delayed-signal compliant! There should be a seal of approval for this kind of thing.

[Side note: there is another problem not related to our discussion, but I must point it out: it is quite possible that QUdpSocket can only accept a finite number of writes at a given time. The signal-based reply leads us to believe that it would probably queue all packets internally and send them as kernel write space becomes available, but a peek at the code shows that writeDatagram() will report errors immediately (synchronously). This likely means it is not queuing at all, and attempting to call PacketSender::start() with a large list of addresses will likely fail.]

Amusingly, writeDatagram() also has a return value, which essentially indicates the same thing as the signal. By looking at the code, we can see that we only need to use either the signals or the return value. However, by looking at only the documentation, it would appear that we may need to check both values (and we probably still should, since we have no guarantee that the writeDatagram() implementation will always work this way). What happens if bytesWritten() is emitted but writeDatagram() returns an error? Talk about confusing…

Really, Trolltech ought to either get rid of the signals or the return value. Getting rid of the signals is the easier solution, but is less abstract as it relies on the underlying socket implementation being able to know the result of the write operation immediately. Getting rid of the return value is a more abstract solution, but something really needs to be done about those non-delayed signals.

My policy when interacting with Qt classes lately is to compensate for these kinds of problems at the earliest level (usually within whichever class of mine is directly using the Qt class), so that code stays clean all the way up. The IrisNet TcpStream and UdpStream classes, for example, internally assume the worst from QTcpSocket and QUdpSocket and supply proper workarounds.