Using synchronized block in ExecutorService

Question

I have the following code snippet:

public class Service<T> {
    private ConcurrentMap<Integer, Integer> locks = new ConcurrentHashMap<Integer, Integer>();
    public final ExecutorService exec = Executors.newFixedThreadPool(5);

    public Future<Boolean> foo(T o, String fileName) throws IOException {
        return exec.submit(new Callable<Boolean>() {
            @Override
            public Boolean call() throws IOException {
                File f = new File(fileName);

//                synchronized (getCacheSyncObject(name.hashCode())) {
//                    try (OutputStream out = new FileOutputStream(f)) {
//                        //some actions
//                    }
//                }

                try (OutputStream out = new FileOutputStream(f)) {
                    //some actions
                }

                return true;
            }
        });
    }

    private Object getCacheSyncObject(final Integer id) {
        locks.putIfAbsent(id, id);
        return locks.get(id);
    }
}

public class Main {
    public static void main(String[] args) throws IOException {
        Object obj = new Object();

        Service<Object> service = new Service<>();
        Future<Boolean> result1 =  service.foo(obj, "filename"); // is there a deadlock?
        service.exec.shutdown();
    }
}

I want to complete one simple task. I need an exclusive file locking. To get this, I put lock on filename of file to synchronized block. But in this case, I don't get anything from foo method. My program does not end.

Answer 1

If you are looking for a lock in scope of Service, you can use its field to store locks. But I'd suggest to don't use fileName.hashCode() because of possible collisions. Use the whole name instead:

import java.io.File;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.OutputStream;
import java.util.concurrent.Callable;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;

class Service<T> {

    private final ConcurrentMap<String, Object> locks = new ConcurrentHashMap<>();
    public final ExecutorService exec = Executors.newFixedThreadPool(5);

    public Future<Boolean> foo(T o,
                               final String fileName) throws IOException {
        return exec.submit(new Callable<Boolean>() {
            @Override
            public Boolean call() throws IOException {
                File f = new File(fileName);

                synchronized (getCacheSyncObject(fileName)) {
                    try (OutputStream out = new FileOutputStream(f)) {
                        out.write("hi".getBytes());
                    }
                }

                return true;
            }
        });
    }

    private Object getCacheSyncObject(final String name) {
        Object result = locks.get(name);

        if (result == null) {
            result = new Object();
            Object prevLock;
            if ((prevLock = locks.putIfAbsent(name, result)) != null) {
                result = prevLock;
            }
        }

        return result;
    }
}

public class Test {

    public static void main(String[] args) throws Exception {
        Object obj = new Object();

        Service<Object> service = new Service<>();
        Future<Boolean> result1 = service.foo(obj, "filename");
        service.exec.shutdown();

        System.out.println("result1 = " + result1.get());
    }
}

BTW, even you have a collision with hashCode(), you can get unexpected output in the file, but if you don't lock anything in that try

try (OutputStream out = new FileOutputStream(f)) {
    out.write("hi".getBytes());
}

you shouldn't get a deadlock. I'd recommend to check your "//some actions"