Reputation: 534
Hi i have extremly simplified a java problem in the following code snippet
public class WhichJavaSynchroIsBestHere {
private BlockingQueue<CustomObject> queue = new PriorityBlockingQueue<CustomObject>();
public void add( CustomObject customO ) {
// The custom objects do never have the same id
// so it's no horizontal concurrency but more vertical one a la producer/consumer
if ( !queue.contains( customO ) ) {
// Between the two statement a remove can happen
queue.add( customO );
}
}
public void remove( CustomObject customO ) {
queue.remove( customO );
}
public static class CustomObject {
long id;
@Override
public boolean equals( Object obj ) {
if ( obj == null || getClass() != obj.getClass() )
return false;
CustomObject other = (CustomObject) obj;
return ( id == other.id;
}
}
}
So this more of producer / consumer problem because presumably the two thread calling add do not pass the same Customobject
(id), but this can happen when if one thread is calling add with same object as a second thread calling remove. The code section in between the if condition and the adding is what seems to me as not thread, safe, i was thinking about the object Lock (no synchronized
blocks) to secure that section, but is a ReadWriteLock
better?
Upvotes: 0
Views: 213
Reputation: 116918
The code section in between the if condition and the adding is what seems to me as not thread-safe
You are correct, it isn't thread-safe. Anytime you have multiple calls to a object, even a synchronized
one, you need to worry about the state of the object changing between the calls if accessed by multiple threads. It's not just than an object might have been removed, but a duplicate object could have been added by a different producer causing duplicates in the queue. The race would be:
The queue would then have 2 copies of A in it.
i was thinking about the object Lock (no synchronized blocks) to secure that section
If you are shying away from synchronized
because of perceived performance problems, then don't. This is a perfect example of where using synchronized
is appropriate. You could get rid of the BlockingQueue
if you are doing all operations inside of a synchronized
block.
is a ReadWriteLock better ?
No because in both cases, the threads are "writing" to the queue. A removal is modifying the queue as much as an add. The ReadWriteLock
allows multiple reading threads if there is no writers but exclusive access to a writing thread. Now the testing of the queue is considered a read but that's not going to save you very much unless there are is a large percentage of times where there are duplicates that are already in the queue.
Also, be very careful of queue.contains(customO)
. Most queues (this includes PriorityBlockingQueue
) run through all items in the queue to look for the one you may be adding (O(N)
). This can be very expensive depending on how many items are in the collection.
This feels to me to be be a good place to use the ConcurrentSkipListSet
. You just do an queue.add()which internally does a put-if-absent. You can do a
queue.pollFirst()` to remove and get the first item. The collection then takes care of the memory synchronization and locking for you and resolves the race conditions.
Upvotes: 0
Reputation: 118794
It would make no difference, as both sections would require a Write lock anyway.
The advantage of a ReadWriteLock is to easily allow multiple Readers that can work with shared access, and, yet, cooperate well with someone who requires exclusive access for Writing.
You could surround the contains
code with a Read lock, and that would make sense if putting in potential duplicates is the bulk of your work. But if it's more a sanity check for a rare edge case, than a primary driver of your work (i.e. the test will pass the vast majority of the time), then there's no reason for the Read lock in this case. Just lock the whole section and be done with it.
Upvotes: 1