2
\$\begingroup\$

Multiple threads write data to a file. When maxLines is reached, the file is closed and a new file is created:

public synchronized void writeLine(String data) throws IOException {
  writer.write(data + "\n");
  lineCounter++;

  if(maxLines != 0 && lineCounter == maxLines) {
    close();

    lineCounter = 0;
    fileCounter++;

    addFile();
    open();
  }
}

As you can see I currently solved this with a synchronized method. This ensures that thread 1 won't write when thread 2 just closed the writer.

I think that's pretty slow. Is there a better solution for this? Maybe with lineCounter as volatile?

\$\endgroup\$
2
  • \$\begingroup\$ What is the writer exactly? \$\endgroup\$
    – kraskevich
    Feb 9, 2015 at 15:52
  • \$\begingroup\$ writer is a BufferedWriter \$\endgroup\$
    – halloei
    Feb 9, 2015 at 15:55

3 Answers 3

2
\$\begingroup\$

Making the lineCounter volatile will not help here. If synchronized is not used, the following sequence of actions is possible:

  1. Thread A writes something to a file.

  2. Thread B writes something to a file.

If the lineCounter was equal to maxLines - 1 before the first step, the behavior of the program is obviously incorrect.

As you have said, you need to synchronize a group of actions(write, close, addFile, open). That's why you need to use synchronized(or a Lock).

About performance: are you sure that synchronization is really an issue? Did you profile your code or are you just making guesses? Before making things more complicated, you should make sure that it will actually solve the problem.

\$\endgroup\$
1
  • \$\begingroup\$ In production I rather use 8 or 16 instead of 2 threads. Without synchronized, I got an Exception from writing to the closed writer pretty fast, which means that many threads normally process writeLine() simultaneously. I didn't profile that, but I think that's proof enough. Isn't it? \$\endgroup\$
    – halloei
    Feb 10, 2015 at 8:30
1
\$\begingroup\$

In addition to why a synchronized block is needed, making the method synchronized is not the best implementation. Using the synchronized modifier on a method synchronizes the method on the class instance. Any code can also synchronize on these instances, potentially causing deadlocks in code you don't control. You should always synchronize on a private final instance that only you control.

\$\endgroup\$
1
\$\begingroup\$

This problem can easily be solved using the Producer-Consumer pattern. Thread A & B from your example are the producers (they produce the tasks : lines to be written to a file). A third thread C, the consumer, will take these tasks, and process them (do the actual writing). Producers hand tasks to the consumer through a thread safe queue.

Typically you'll want a bounded and blocking queue. This way producers will stop producing tasks when the consumer cant' keep up, and the consumer will stop processing when the producers can't keep up.

Synchronization concerns are limited to using a thread safe queue implementation and making sure the tasks are thread safe. As String is immutable that is not a problem, and the JDK comes with various flavours of thread safe queues, so no need to roll your own. (ArrayBlockingQueue should serve just fine, for instance).

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.