Racecondition in PacketWriter

Smack Smack Dev
1

Hello,

Context

We are developing an application with Smack 3.4.1.

In this application we have sevaral XMMPconnections,

that are used by many threads to send packets. (An estimated total 300.000 packets a minute. )

We think that we have encoutered possible racecondition in

org.jivesoftware.smack.PacketWriter

Scenario

A XMPPConncetion creates a PacketWriter.

The PacketWriter:

-> Uses an ArrayBlocklingQueue to manage the packets that need to be written (with a max size of 500).

-> starts a thread “Smack Packet Writer (” + connection.connectionCounterValue + ") ".

This thread use the method

org.jivesoftware.smack.PacketWriter.nextPacket()

to get the next packet.

/**
     * Returns the next available packet from the queue for writing.
     *
     * @return the next packet for writing.
     */
    private Packet nextPacket() {
        Packet packet = null;
        // Wait until there's a packet or we're done.
        while (!done && (packet = queue.poll()) == null) {
            try {
                synchronized (queue) {
                    queue.wait();
                }
            }
            catch (InterruptedException ie) {
                // Do nothing
            }
        }
        return packet;
   }

Other threads can use

org.jivesoftware.smack.PacketWriter.sendPacket(Packet)

(through to sendPacket(packet) method of XMPPConncection) to add packages to the queue.

/**
     * Sends the specified packet to the server.
     *
     * @param packet the packet to send.
     */
    public void sendPacket(Packet packet) {
        if (!done) {
            // Invoke interceptors for the new packet that is about to be sent. Interceptors
            // may modify the content of the packet.
            connection.firePacketInterceptors(packet);             try {
                queue.put(packet);
            }
            catch (InterruptedException ie) {
                log.log(Level.SEVERE, "Failed to queue packet to send to server: " + packet.toString(), ie);
                return;
            }
            synchronized (queue) {
                queue.notifyAll();
            }             // Process packet writer listeners. Note that we're using the sending
            // thread so it's expected that listeners are fast.
            connection.firePacketSendingListeners(packet);
        }
    }

Now, i think the following scenario is possible :

-> the queue.poll() method in nextpacket() returns null

-> context switches happen and the queue gets 500 queue.put(packet) in sendPacket(packet) from different threads

-> the 501the queue.put(packet) blocks ( the size of the queue is 500)

-> nextpacket() continues and starts waiting on a notification

-> a notification never happens because all the following queue.put(packet) are blocked…

We have encoutered a waiting smack writer (*) thread while we can verify , with a heap dump, that the queue in this writer thread is full…

Possible solution

In:**
**

org.jivesoftware.smack.PacketWriter.nextPacket()**
**

  1. use a timed wait to ait for a notification

or (maybe better)

  1. Use ArrayBlockingQueue.take() instead of ArrayBlockingQueue.poll()

I am relativly new to multi threading .

I hope i am on the correct path to pinpoint a possible bug and a solution.

If not, please correct me. :wink:

regards,

m_dh

2

Good catch. Getting concurrency right is usually not easy.

I can image that poll() was used instead of take() because of the done boolean, ie. they wanted the non-blocking semantic of poll() here.

One could increase the synchronized block for both methods so that includes the put and poll methods. Of course, then you wouldn’t need any concurrent datastructe and could simply use a Queue.

I could image there is a better solution.

3

SMACK-560

We decided to try the clean approach with take() and interrupt().

4

Great, thanks!

regards,

m_dh