Is this the correct way of using synchronized in Java?

Question

The for loop shown here is run within a thread. Inside the sychronized block, a thread writes to some file. There are several different files, so the writers are kept in an array. What I want to make sure here is that no two different threads are writing to the same file at the same time. However, they can write to different files. Am I using the correct parameter with the synchronized block?

for(Element e: elements) { int i = getWriterIndex(e) writeri = writers(i) synchronized(writeri) { // Write to corresponding segment writers(i).write(e) recordsWritten(i) += 1 } }

Can you include the method containing this synchronized block? — Tim Biegeleisen
– Tim Biegeleisen, Commented Jul 9, 2016 at 11:59
I've added it. Not the method, but the loop from where its called. — pythonic
– pythonic, Commented Jul 9, 2016 at 12:13
I'm no pro in this, but this kind of looks producer-consumer to me. Your synchronized code will block the entire for loop. Perhaps you want to use a queue for each file and send the data to the respective queue so that they can be written to the respective file in its own thread. Again, I'm no expert in this, not at all. — Hovercraft Full Of Eels
– Hovercraft Full Of Eels, Commented Jul 9, 2016 at 12:15
Why would it block the whole loop? It would only block if the threads are writing to the same writer (if i is the same). No? — pythonic
– pythonic, Commented Jul 9, 2016 at 12:18
The for loop is being run in a single thread. If the synchronization blocks a writer, then the whole for loop blocks. — Hovercraft Full Of Eels
– Hovercraft Full Of Eels, Commented Jul 9, 2016 at 12:21

stikku · Accepted Answer · 2016-07-09 13:38:28Z

While I think that this would work, I strongly suggest that you avoid using synchronized. The reason being that it is quite common that you end up being to strict in your synchronization policy. As others have mentioned this seems to be a perfect use case for queues.

If you do not want to use queues in most scenarios (including this) I would suggest using locks in order to maintain thread safety (typically ReentrantReadWriteLock). You can find an example here

In your case I would create a single lock per writer and require that in order to use the writer the current thread must hold the writelock. (If you are only writing you might use simple locks instead of ReentrantReadWriteLock.

Dimos · Accepted Answer · 2016-07-09 12:34:43Z

Yes, your synchronisation code will work as expected, as long as the synchronised block accesses only the data structure that is used as the lock. So, in your code, writeri cannot be accessed by multiple threads concurrently, so it is thread-safe.

However, you have to make sure that you are not accessing the variable recordsWritten somewhere else, because then you will have race-conditions. So, ideally you would also lock on that variable as well (in every place you access it), or you can use some Java primitive such as AtomicInteger

Collectives™ on Stack Overflow

Is this the correct way of using synchronized in Java?

2 Answers 2

Comments

Comments

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

Comments

Related