Code Review · Java

Java Code Review: Resource Leak with Streams and Files

Review Java code that reads a file using a stream but forgets to close the underlying resource.

Code ReviewJavaFilesStreamsResource Management

Code Review Exercise

Review the following Java code. The method reads a log file and returns all lines that contain an error marker.

java

import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.List;
import java.util.stream.Collectors;
import java.util.stream.Stream;

public class LogReader {
    public List<String> readErrors(String filePath) throws IOException {
        Stream<String> lines = Files.lines(Path.of(filePath));

        return lines
                .filter(line -> line.contains("ERROR"))
                .collect(Collectors.toList());
    }
}

What issues would you mention in a code review?

Review Findings

Issue #1: The stream is not closed

Files.lines(...) returns a stream backed by an open file resource. This stream should be closed after use. In this code, the method returns without explicitly closing it.

Issue #2: Resource leaks can be subtle

The method may appear to work in small tests, but repeated calls can leave file handles open longer than expected. In production, this can eventually cause failures such as too many open files.

Issue #3: Exception paths matter

If an exception occurs while filtering or collecting the lines, the stream still needs to be closed. Manual cleanup is easy to forget, which is why try-with-resources is the safer pattern.

Issue #4: Missing input validation or contract

The method accepts a raw String path and does not document whether null, missing files, or unreadable files are expected. That may be fine if callers handleIOException, but the contract should be clear.

Why This Happens

Not every Java stream owns an external resource. A stream created from a list usually does not need special cleanup.

java

List<String> names = List.of("Alice", "Bob");
names.stream().filter(name -> name.startsWith("A"));

But a stream created from a file is different. It is connected to an underlying file resource, so it should be closed.

java

Stream<String> lines = Files.lines(Path.of("app.log"));

Fixed Version: try-with-resources

The cleanest fix is to put the stream inside atry-with-resources block.

java

import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.List;
import java.util.stream.Collectors;
import java.util.stream.Stream;

public class LogReader {
    public List<String> readErrors(String filePath) throws IOException {
        try (Stream<String> lines = Files.lines(Path.of(filePath))) {
            return lines
                    .filter(line -> line.contains("ERROR"))
                    .collect(Collectors.toList());
        }
    }
}

Now the stream is closed whether the method succeeds or an exception is thrown.

Alternative: Read All Lines

If the file is small enough, another simple option is to read all lines first. This avoids returning or storing a stream entirely.

java

public List<String> readErrors(String filePath) throws IOException {
    return Files.readAllLines(Path.of(filePath))
            .stream()
            .filter(line -> line.contains("ERROR"))
            .collect(Collectors.toList());
}

This is easier to read, but it loads the whole file into memory. For large files, Files.lines(...) withtry-with-resources is usually better.

Better Review Comment

In an interview, avoid saying only:

This leaks resources.

A stronger review comment explains the issue and gives a fix:

Files.lines() creates a stream backed by a file. I would wrap it in try-with-resources so the file handle is closed even if filtering or collecting throws an exception.

Interview Follow-Ups

Do all Java streams need to be closed?

No. Streams backed by external resources, such as files or sockets, should be closed. Streams over in-memory collections usually do not need special cleanup.

Why is try-with-resources preferred?

It closes the resource automatically when the block exits, including when an exception is thrown. This avoids fragile manual cleanup.

Is Files.readAllLines always better?

No. It is simple for small files, but it loads the whole file into memory. For large files, streaming lines can be more memory-efficient.

What is the broader review lesson?

Look for resources that must be released: files, streams, sockets, database connections, HTTP responses, and similar objects.

What Interviewers Expect

This exercise tests whether you notice lifecycle problems, not just syntax issues.

Correctness: file-backed streams should be closed.
Reliability: leaks may only appear after repeated production usage.
Maintainability: try-with-resourcescommunicates ownership clearly.
Tradeoff: readAllLines() is simpler, but streaming is better for large files.