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();
                });
    }
}

CodePudding user response：

You are only synchronizing the method not the collection.

In HashSet javadocs:

Note that this implementation is not synchronized. If multiple threads access a hash set concurrently, and at least one of the threads modifies the set, it must be synchronized externally. This is typically accomplished by synchronizing on some object that naturally encapsulates the set. If no such object exists, the set should be "wrapped" using the Collections.synchronizedSet method. This is best done at creation time, to prevent accidental unsynchronized access to the set:

Set s = Collections.synchronizedSet(new HashSet(...));

You need to create synchronization logic (like using a Mutator, Semaphore, Lock, or other mechanism) to avoid the problem.

Reference:

• https://docs.oracle.com/javase/7/docs/api/java/util/HashSet.html
• Java HashSet Synchronization query
• (Java) Why is a HashSet allowed to be used synchronously if it is non synchronized?
• https://www.baeldung.com/java-concurrent-hashset-concurrenthashmap
• https://knpcode.com/java/collections/how-to-synchronize-hashset-in-java/

CodePudding user response：

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();
                });

    }
}