Why synchronized in Java doesn't work properly?

Question

I have a task to do a synchronized Set using synchronized key word. Here's my realization

import java.util.Collection;
import java.util.HashSet;
import java.util.Iterator;
import java.util.Set;

public class SyncSet<T> implements Set<T> {
    Set<T> set;

    public SyncSet() {
        set = new HashSet<>();
    }

    public SyncSet(int size) {
        set = new HashSet<>(size);
    }

    public SyncSet(Collection<T> collection) {
        set = new HashSet<>(collection);
    }

    @Override
    public synchronized int size() {
        return set.size();
    }

    @Override
    public synchronized boolean isEmpty() {
        return set.isEmpty();
    }

    @Override
    public synchronized boolean contains(Object o) {
        return set.contains(o);
    }

    @Override
    public synchronized Iterator<T> iterator() {
        return set.iterator();
    }

    @Override
    public synchronized Object[] toArray() {
        return set.toArray();
    }

    @Override
    public synchronized <T1> T1[] toArray(T1[] a) {
        return set.toArray(a);
    }

    @Override
    public synchronized boolean add(T t) {
        return set.add(t);
    }

    @Override
    public synchronized boolean remove(Object o) {
        return set.remove(o);
    }

    @Override
    public synchronized boolean containsAll(Collection<?> c) {
        return set.containsAll(c);
    }

    @Override
    public synchronized boolean addAll(Collection<? extends T> c) {
        return set.addAll(c);
    }

    @Override
    public synchronized boolean retainAll(Collection<?> c) {
        return set.retainAll(c);
    }

    @Override
    public synchronized boolean removeAll(Collection<?> c) {
        return set.removeAll(c);
    }

    @Override
    public synchronized void clear() {
        set.clear();
    }
}

But when I run this code a lot of times, then sometimes it doesn't work like a synchronized set. For example, if i put 1000 different elements in it in one thread, and then delete all 1000 elements in another thread. And repeat it 1000 times, the set wouldn't be empty in a few times. Here's how I use it

public class Main {
    private static void task1() {
        Set<Integer> testSet = new SyncSet<>();
//      Set<Integer> testSet = new HashSet<>();

        Thread t1 = new Thread(() -> {
            for (int i = 0; i < 1000; ++i) {
                testSet.add(i * 10);
            }
        });

        Thread t2 = new Thread(() -> {
            for (int i = 0; i < 1000; ++i) {
                testSet.remove(i * 10);
            }
        });

        t1.start();
        t2.start();

        try {
            t1.join();
            t2.join();
        } catch (InterruptedException e) {
            e.printStackTrace();
        }

        System.out.println(testSet.size());
    }

    public static void main(String[] args) {
        IntStream.range(0, 1000)
                .forEach(el -> {
                    System.out.print(el + ": ");
                    task1();
                });
    }
}

Without seeing the code that's using it, it's hard to know what's going on. Please provide a [mcve]. But bear in mind that making the `iterator()` method synchronized doesn't really protect you - that method will complete pretty much immediately, but then the *use* of the iterator isn't synchronized. — Jon Skeet, Feb 14 '22 at 09:48
My guess is a race condition; items are removed before they are added. — pveentjer, Feb 14 '22 at 09:52
If you add elements in one thread and simultaneously try to remove them in another, there is no guarantee that the element you try to remove is in the set when you try and remove it. Your synchronized methods do not help that. — khelwood, Feb 14 '22 at 09:52
You can't be sure that `t1` already added the elements that `t2` tries to remove. — maloomeister, Feb 14 '22 at 09:52
The implementation is fine. The test is wrong. There is no happens-before relationship between the add and remove. In the worst case, the scheduler can run the entire of t2 before t1 ever starts. In which case, the set will have all 1000 elements. FWIW if you just want add synchronized to every method, you can write `Collections.synchronizedSet(new HashSet<>())`. It's the same as yours, but less code — Michael, Feb 14 '22 at 09:54
@JonSkeet And FWIW Collections.synchronizedSet has the same issue. The javadoc specifically says to sync on the set instance if you want a thread-safe iterator. — Michael, Feb 14 '22 at 09:58
@Michael I assume you mean happens-before relation of the Java Memory Model. In that case, there are happens-before edges because every method is synchronized. — pveentjer, Feb 14 '22 at 11:58

score 1 · Answer 1 · answered Feb 14 '22 at 10:04

even is value is not there in set testSet.remove(i * 10); it will try to remove the value make sure you are removing after value is added.

In below code this line makes sure that value is deleted after its in the set. This is not the recommended approach but it will give you some direction so you could fix you code.

        while (true) {
            if (!testSet.contains(i * 10))
                continue;
            testSet.remove(i * 10);
            break;
        }

Code:

import java.util.Collection;
import java.util.HashSet;
import java.util.Iterator;
import java.util.Set;
import java.util.stream.IntStream;

class SyncSet<T> implements Set<T> {
    Set<T> set;

    public SyncSet() {
        set = new HashSet<>();
    }

    public SyncSet(int size) {
        set = new HashSet<>(size);
    }

    public SyncSet(Collection<T> collection) {
        set = new HashSet<>(collection);
    }

    @Override
    public synchronized int size() {
        return set.size();
    }

    @Override
    public synchronized boolean isEmpty() {
        return set.isEmpty();
    }

    @Override
    public synchronized boolean contains(Object o) {
        return set.contains(o);
    }

    @Override
    public synchronized Iterator<T> iterator() {
        return set.iterator();
    }

    @Override
    public synchronized Object[] toArray() {
        return set.toArray();
    }

    @Override
    public synchronized <T1> T1[] toArray(T1[] a) {
        return set.toArray(a);
    }

    @Override
    public synchronized boolean add(T t) {
        return set.add(t);
    }

    @Override
    public synchronized boolean remove(Object o) {
        return set.remove(o);
    }

    @Override
    public synchronized boolean containsAll(Collection<?> c) {
        return set.containsAll(c);
    }

    @Override
    public synchronized boolean addAll(Collection<? extends T> c) {
        return set.addAll(c);
    }

    @Override
    public synchronized boolean retainAll(Collection<?> c) {
        return set.retainAll(c);
    }

    @Override
    public synchronized boolean removeAll(Collection<?> c) {
        return set.removeAll(c);
    }

    @Override
    public synchronized void clear() {
        set.clear();
    }
}

public class SyncMain {
    private static void task1() {
        Set<Integer> testSet = new SyncSet<>();
//      Set<Integer> testSet = new HashSet<>();
        Thread t1 = new Thread(() -> {
            for (int i = 0; i < 1000; ++i) {
                testSet.add(i * 10);
            }
        });

        Thread t2 = new Thread(() -> {
            for (int i = 0; i < 1000; ++i) {
                while (true) {
                    if (!testSet.contains(i * 10))
                        continue;
                    testSet.remove(i * 10);
                    break;
                }
            }
        });

        t1.start();
        t2.start();

        try {
            t1.join();
            t2.join();
        } catch (InterruptedException e) {
            e.printStackTrace();
        }

        System.out.println(testSet.size());
    }

    public static void main(String[] args) {
        IntStream.range(0, 1000)
                .forEach(el -> {
                    System.out.print(el + ": ");
                    task1();
                });

    }
}

Why synchronized in Java doesn't work properly?

1 Answers1

Code: