CODEWITHSUNDEEP
javaconcurrency
Pavel
Pavel

Reputation: 2101

ReadWriteLock for concurrency access to file

I have a class which implements read and write operation to file in a concurrent environment. I know BufferedInputStream and BufferedWriter are synchronized but in my case read and write operations can be used simultaneously. Now I use ReentrantReadWriteLock but I'm not confident about a solution correctly.

public class FileSource {

    private final File file;

    private final ReadWriteLock lock;

    public FileWrapper(final File file) {
        if (Objects.isNull(file)) {
            throw new IllegalArgumentException("File can't be null!");
        }
        this.file = file;
        this.lock = new ReentrantReadWriteLock();
    }

    public String getContent() {
        final Lock readLock = lock.readLock();
        readLock.lock();

        final StringBuilder sb = new StringBuilder();
        try (final BufferedInputStream in =
                     new BufferedInputStream(
                             new FileInputStream(file))) {
            int data;
            while ((data = in.read()) > 0) {
                sb.append(data);
            }
        } catch (IOException e) {
            e.printStackTrace();
        } finally {
            readLock.unlock();
        }
        return sb.toString();
    }

    public void saveContent(final String content) {
        final Lock writeLock = lock.writeLock();
        writeLock.lock();

        try (BufferedWriter out =
                     new BufferedWriter(
                             new FileWriter(file))) {
            out.write(content);
        } catch (IOException e) {
            e.printStackTrace();
        } finally {
            writeLock.unlock();
        }
    }
}

ReentrantReadWriteLock is the correct solution in this case or I need to use ReentrantLock or something else? (with a reason)

This discussion not about class design like File as a state or send File directly in the method or using nio package or ext. It shouldn't be a utility class. Method signatures and File as a field must stay without changes. It is about potential concurrency problems with File and InputStream\OutputStream.

Upvotes: 1

Views: 110

Answers (1)

rzwitserloot
rzwitserloot

Reputation: 102804

RRWL is fine here. Of course, if some code makes a new FileWrapper("/foo/bar.txt") and then some other code also makes a separate new FileWrapper("/foo/bar.txt") those two wrappers will be falling all over themselves and will cause things to go pearshaped; I assume you have some external mechanism to ensure this cannot happen. If you don't, some take on ConcurrentHashMap and its concurrency methods (such as computeIfAbsent; don't use the plain jane get/put for these) can help you out.

Note that your exception handling is bad. Exception messages should not end in punctuation (think about it: Without this rule, 80% of all exception messages would end in an exclamation mark and will make log files a fun exercise), and in general, if you ever write catch (Exception e) { e.printStackTrace(); }, you go to that special place reserved for people who talk in movie theaters, and people who write that.

I'd say a method called saveContent is justified in throwing some checked exception; after all, it's rather obvious that can fail, and code calling it can feasibly be expected to perhaps take some action if it does.

If you just can't get there, the proper ¯\(ツ)/¯ I dunno catch block handler is: throw new RuntimeException("uncaught", e);. Not e.printStackTrace();. The latter logs to an uncontrollable place, shaves off useful information, and crucially, keeps on running as if nothing is wrong, silently ignoring the fact that a save call just failed, whereas the former preserves all and will in fact abort code execution. It makes it hard to recover, but at least it's easier than e.printStackTrace, and if you wanted it to be easier, than make a special exception. Or just throw that IOException unmolested (you get way shorter code to boot!).

Another insiduous bug in this code is that it uses 'platform default charset encoding' to read your file, which is very rarely what you want.

The new Files API can also read the entire file in one go, saves you a ton of code on that read op. As you upgrade your code, you get the benefit of the Files API's unique take on charset encodings: Unlike most other places in the java libraries, java.nio.file.Files will assume you meant to use UTF-8 encoding if you fail to specify (instead of 'platform default', i.e. 'the thing that you cannot test for that will blow up in production and waste a week of your time chasing after it').

Upvotes: 1

Related Questions