0

I have the following code as an OSGi module.

When it runs, I get the message that the logger has been set:

UdpListener > setStoreLog: 'com.mine.logger.internal.storeindb.StoreLog@1c6f579'

But immediatly after that, the loop in the run() function says that storeLog is empty

ERROR > UdpListener > run > storeLog is not available.

Any ideas what could be wrong?

Could it be the fact that this is running in a thread?

package com.mine.logger.internal.udp;

import java.io.IOException;
import java.net.DatagramPacket;
import java.net.DatagramSocket;
import java.net.InetAddress;
import java.net.SocketException;
import java.util.Date;

import com.mine.logger.storeindb.IStoreLog;

public class UdpListener extends Thread
{
    private int port;

    private IStoreLog storeLog;

    public void setStoreLog(IStoreLog value)
    {
        this.storeLog = value;
        System.out.println("UdpListener > setStoreLog: '" + this.storeLog.toString() + "'");
    }

    public void unsetStoreLog(IStoreLog value)
    {
        if (this.storeLog == value) {
            this.storeLog = null;
        }
        System.out.println("UdpListener > unsetStoreLog");
    }

    public UdpListener() 
    {
        // public, no-args constructor needed for Declarative Services !!!
    }

    public UdpListener(int port) 
    {
        this.port = port;
    }

    public void run()
    {
        startListener();
    }

    private void startListener()
    {
        try {
            // send command
            DatagramSocket socket = new DatagramSocket(port);

            while (true)
            {
                byte[] b = new byte[1000];
                DatagramPacket recvdPacket = new DatagramPacket(b, b.length);
                socket.receive(recvdPacket);

                System.out.println("UdpListener: Packet received. " + (new String(b)));

                try
                {
                    if (this.storeLog != null)
                        this.storeLog.doStore(new Date(), InetAddress.getByName("0.0.0.0"), port, 1, "UDP", b);
                    else
                        System.err.println("ERROR > UdpListener > run > storeLog is not available.");
                }
                catch (Exception e)
                {
                    System.err.println("ERROR > UdpListener > run > storeLog > Exception: " + e.toString());
                }
            }
        } catch (SocketException e) {
            System.out.println("ERROR > UdpListener > run > SocketException: " + e.getMessage());
        } catch (IOException e) {
            System.out.println("ERROR > UdpListener > run > IOException: " + e.getMessage());
        } catch (Exception e) {
            System.out.println("ERROR > UdpListener > run > Exception: " + e.getMessage());
        }
    }
}
stacker
  • 68,052
  • 28
  • 140
  • 210
Frank
  • 5,741
  • 4
  • 28
  • 49

2 Answers2

2

Your code is not thread safe. The storeLog field is read and written by multiple threads without any synchronization. If you have a mutable field that is read and written by more than one thread, you must ensure the field is always safely accessed for both read and write. I highly recommend the excellent book Java Concurrency in Practice http://www.javaconcurrencyinpractice.com/ for anyone writing Java code.

BJ Hargrave
  • 9,324
  • 1
  • 19
  • 27
-2

solved by moving storelog to a seperate class

Frank
  • 5,741
  • 4
  • 28
  • 49
  • 1
    i don't think you should solve a problem by moving code around. the best way is to understand the why then you don't have to face this battle again. Java concurrency is tricky at best, but like Regex, once you understand it your life will be significantly better. – Nico Jan 24 '12 at 19:05