CODEWITHSUNDEEP
javaxmlmultithreading
Alosyius
Alosyius

Reputation: 9121

Java thread safe write to xml file?

I have the following method to save new data to an xml file. It stores chat history:

public void addMessage(String from, String agentName, String msg, String time, String channel){

    try {
        DocumentBuilderFactory docFactory = DocumentBuilderFactory.newInstance();
        DocumentBuilder docBuilder = docFactory.newDocumentBuilder();
        org.w3c.dom.Document doc = docBuilder.parse(filePath);

        Node data = doc.getFirstChild();

        org.w3c.dom.Element root = doc.createElement(channel);
        org.w3c.dom.Element message = doc.createElement("message");
        org.w3c.dom.Element _sender = doc.createElement("sender"); _sender.setTextContent(from);
        org.w3c.dom.Element _content = doc.createElement("content"); _content.setTextContent(msg);
        org.w3c.dom.Element _recipient = doc.createElement("recipient"); _recipient.setTextContent(agentName);
        org.w3c.dom.Element _time = doc.createElement("time"); _time.setTextContent(time);


        message.appendChild(_sender); message.appendChild(_content); message.appendChild(_recipient); message.appendChild(_time);
        root.appendChild(message);

        data.appendChild(root);

        TransformerFactory transformerFactory = TransformerFactory.newInstance();
        Transformer transformer = transformerFactory.newTransformer();
        DOMSource source = new DOMSource(doc);
        StreamResult result = new StreamResult(new File(filePath));
        transformer.transform(source, result);

   } 
       catch(Exception ex){
    System.out.println("Exceptionmodify xml");
   }
}

The problem all a sudden i am getting is the exception Exceptionmodify xml is being trown. I am guessing this is because i am accessing this method from a number of different threads and it messes up the xml-file.

Any ideas how i could make this thread safe?

Upvotes: 1

Views: 1559

Answers (3)

extraneon
extraneon

Reputation: 23960

It seems that all the messages are written to a single File. Access to a file should be single threaded to ensure the various messages do not get mingled.

As other answers have shown, marking all methods which access that file with synchronized is a solution. And it could be the correct solution if the addMessage would actually return something.

But since addMessage does not return anything, it is not necessary for the callers to wait until it is their turn to write a message.

In this case, a producer-consumer pattern might be more appropriate. This is typically implemented by having a queue shared in all the producers (those who call addMessage), and a single thread which reads from that queue and writes the message to the file.

A nice queue implementation would be: BlockingQueue. Check out the javadoc as it shows the producer-consumer logic!

If you were to change the current implementation to a queue-based implementation with the least amount of work, that would go something like this:

An object to contain the info for the XML message, so it can be put on the queue:

public class MessageInfo {
    private String from;
    private String agentName;
    private String msg;
    private String time;
    private String channel;
    public MessageInfo(String from, String agentName, String msg, String time, String channel) { // this.from = from; // etc.
    }

    // getters 
}

Your class, with the addMessage changed so it puts the data on the queue

public class Yours {
    private Queue<MessageInfo> messageQueue;

    public Yours(Queue<MessageInfo> queue) {this.messageQueue = queue;}

    public void addMessage(...) {
        MessageInfo info = new MessageInfo(...);
        try {
            messageQueue.offer(info);
        } catch (InterruptedException e) {
            System.out.println("Could not put message on queue " + info);
        }
    }
}

A class which reads from the queue and writes to a file. There may be only 1 per file, so perhaps the filename should be part of the constructor.

public class MessageWriter implements Runnable {
   private Queue<MessageInfo> messageQueue;
   public MessageWriter(Queue queue) { this.messageQueue = queue; }
   public void run() {
      try {
          while(true) { consume(queue.take()); }
          } catch (InterruptedException ex) { ... handle ...
          }
   }
   void consume(MessageInfo info) { 
       // your writing logic, moved from addMessage to here
   }
}

And putting it together:

public class Main {
    public static void main(String[] args) {
        // 20 messages may be on the queue to handle spikes in demand
        // after that, offer() will wait until there is room for the next
        Queue queue = new ArrayBlockingQueue<MessageInfo>(20); 

        Yours = new Yours(queue);
        MessageWriter consumer = new MessageWriter(queue);

        // Start the consumer
        new Thread(consumer).start(); // Perhaps an executor service would be better

        // Start your threads the way you did before
        ...

    }
}

Upvotes: 1

Rakesh Soni
Rakesh Soni

Reputation: 10887

Use synchronized method. like below

public synchronized void addMessage(String from, String
                           agentName, String msg, String time, String channel)
{
   .....
}

Upvotes: 2

VWeber
VWeber

Reputation: 1261

You should synchronize your methods.

Here is an example:

public class SynchronizedCounter {
    private int c = 0;

    public synchronized void increment() {
        c++;
    }

    public synchronized void decrement() {
        c--;
    }

    public synchronized int value() {
        return c;
    }
}

Upvotes: 3

Related Questions