13

I am trying to write some content to a file, through multiple threads in Java. Each thread reads a different input file, does some computation and writes some (different) content to the common output file. The problem is that in the end, the output file only contains the content written by the last terminating thread and not the content from the other threads. Relevant code for the threads -

public void run()
{
    try
    {
        File file = new File("/home/output.txt");
        if (!file.exists()) 
        {
             file.createNewFile();
        }
        FileWriter fw = new FileWriter(file.getAbsoluteFile());
        BufferedWriter bw = new BufferedWriter(fw);

        BufferedReader br = new BufferedReader(new FileReader(inputfile)); // each thread reads a different input file
        String line="";

        while((line=br.readLine())!=null)
        {
            String id = line.trim();               // fetch id

            StringBuffer sb = processId(userId);   // process id

            synchronized(this){
            bw.write(sb.toString() + "\n");        // write to file
            }
        }
        bw.close();
    }
    catch (IOException e)
    {
        e.printStackTrace();
    }
}

How do i make all threads write their content to the common file ?

Ankit Rustagi
  • 5,539
  • 12
  • 39
  • 70
  • 1
    You have to append to the existing file (``FileOutputStream`` has a boolean flag for that). But you would also have to make the writing ``synchronized``, to make sure only one Thread writes at the same time. – qqilihq Oct 06 '13 at 10:37
  • 1
    Don't use StringBuffer if you can use StringBuilder. In this case processId() can return a String. – Peter Lawrey Oct 06 '13 at 12:00
  • 3
    You need to open the file once across all thread and co-ordinate their writing. Otherwise you are likely to get a jumbled mess. I suggest you have a single threaded executor and submit tasks to it to write to the file. This will ensure single threaded writing. – Peter Lawrey Oct 06 '13 at 12:01
  • 1
    I am using synchronized blocks, please se my edit. would it still cause problems ? – Ankit Rustagi Oct 06 '13 at 12:56

5 Answers5

23

Use the constructor for FileWriter that uses append mode

FileWriter fw = new FileWriter(file.getAbsoluteFile(), true);
Reimeus
  • 158,255
  • 15
  • 216
  • 276
  • 1
    If append as true, bytes will be written to the end of the file rather than the beginning.So without mentioning it also, it should work (in that bytes should be written at beginning). i don'e see any issue in OP. – M Sach Oct 06 '13 at 10:44
10

Have a single file writer thread which keeps reading from blocking queue and writing to a file. All other 20 threads will just put the data in blocking queue. This will prevent contention between 20 writer threads.

jayesh
  • 367
  • 3
  • 15
3

Few points related to your code :

1. The way you are creating the FileWriter is not correct . If you want to append data to the file, use the constructor that contains an additional boolean argument ( Make it true ):

public FileWriter(File file,boolean append) throws IOException

For example :

FileWriter fw = new FileWriter(file.getAbsoluteFile(),true);

2. You are talking about multiple threads that will share a common file , but I couldn't see any synchronized block in your code . Use synchronization to ensure that only one thread can access the shared resource at a time.

Tuna
  • 2,937
  • 4
  • 37
  • 61
Java World
  • 71
  • 1
  • 2
    Hi , I think you should take a lock on BufferedWriter and not this. Lets say there are two threads named Thread-A and Thread-B tries to write to the file . Now both will try to take a lock in this instance , but they should take lock on the shared resource that is "bw" in above program. What are your thoughts on this ? – Java World Oct 06 '13 at 13:02
  • The BufferedWriter is being created in every thread as a local variable in the run() method. So synchronizing on it is not going to help either. – eternal_learner Jun 12 '22 at 12:12
2

FileWriter fw = new FileWriter(file.getAbsoluteFile(),true);

should be used which indicates append mode.

harpun
  • 4,022
  • 1
  • 36
  • 40
Anuj Aneja
  • 1,323
  • 11
  • 13
-5

I'm a newbie. SO I MIGHT BE WRONG! But your code MAY or MAY NOT work. Is that the run of a Runnable or thread? Note this:

MyClass implements Runnable {

 public void run() {
 ....synchronized(this)..
 }

}

Thread t1 = new Thread(new MyClass());
Thread t2 = new Thread(new MyClass());

Many of us do it like that(I did it, do it:), and yey! Each thread will obtain a lock on different obj, and you can end up with unexpected results, unless the OS uses some mechanism to sync writes to the same file (I don't know). If it does, then your synchonized is useless(useless anyway in my example), if it doesn't you're pretty.... Also as a note, there may be another process using this output.txt of which we have no idea...complicated stuff, at least for me

My opinion is that synchronized is useful if you have one instance of the class which SHARES, MUTABLE, STATE. (Let's forget about synchronized and static for now)

Does MyClass have state? No. Is it shared? NO, not in my example. Will it work? Not like this. Probably like this?

MyClass mc = new MyClass()
Thread t1 = new Thread(mc);
Thread t2 = new Thread(mc);

PS: I forgot to call start on every thread.

dalvarezmartinez1
  • 1,385
  • 1
  • 17
  • 26