CODEWITHSUNDEEP
javamultithreadingsynchronization
jokarl
jokarl

Reputation: 2225

What map methods must be synchronized for thread safety?

I have a class which contains a map and this map might be accessed simultaneously from several threads.

I create my class as follows:

@Autowired
public Scheduler() {
    this.scheduledRunnables = Collections.synchronizedSortedMap(new TreeMap<Integer, Runnable>());
}

So according to the documentation, to ensure this works correctly I have to follow these guidelines:

Returns a synchronized (thread-safe) sorted map backed by the specified sorted map. In order to guarantee serial access, it is critical that all access to the backing sorted map is accomplished through the returned sorted map (or its views). It is imperative that the user manually synchronize on the returned sorted map when iterating over any of its collection views, or the collections views of any of its subMap, headMap or tailMap views.

I am asking this question because I am not entirely sure if I fulfill the manual synchronization part, or if I use the block unnecessarily.

I am accessing this map in two differents ways.

Accessing lowest key value

So this one I'm 99% sure I should use the synchronized block as it is an example in the documentation when accessing the iterator.

public int getNextAvailableExecutionOrder() {
    synchronized (scheduledRunnables) {
        if (scheduledRunnables != null && scheduledRunnables.size() > 0) {
            return scheduledRunnables.keySet().stream().min(Comparator.comparing(Integer::valueOf)).get() + 1;
        }
        return Ordered.HIGHEST_PRECEDENCE + 1;
    }
}

Adding new runnables

This one I'm a bit unsure of. Is the maps iterator accessed under the hood here, or can I safely put new items in it without the synchronized block?

protected void scheduledNewTask(final int order, final Runnable task) {
    LOG.info("Added new scheduled task {} with order {}", taskName, order);
    synchronized (scheduledRunnables) {
        scheduledRunnables.put(order, task);
    }
}

Edit

The above is my current implementation, but it does not seem to work as I want it to:

2019-02-20 08:33:39.080  INFO 32082 --- [       Thread-6] s.i.s.a.gatemaster.SchedulingSupport     : Added new scheduled task Log upload with order -2147483648
2019-02-20 08:33:39.838  INFO 32082 --- [tHubReceiveTask] s.i.s.a.gatemaster.SchedulingSupport     : Added new scheduled task Firmware update (1.9.1) with order -2147483648

Upvotes: 1

Views: 128

Answers (1)

HPCS
HPCS

Reputation: 1454

If this what you posted are the only two cases where you accessing/manipulating the map, then you don't need to wrap the map with the synchronized wrapper with Collections.synchronizedSortedMap, because you synchronized all the accesses to the map manually so you don't need the wrapper.

To your question, if you want to use Collections.synchronizedSortedMap. The second access where you put something to the map you don't have to manually synchronized because the put method is already synchronized by the synchronizedSortedMap wrapper. You correctly synchronized the access to the map in getNextAvailableExecutionOrder method, because indeed stream iterate the map and you need to manualy synchronized this, like mentioned in the javadoc.

Upvotes: 1

Related Questions