Fix resource leak in FileBackedOutputStream to prevent file handle exhaustion

[lmcrean]

Problem

FileBackedOutputStream leaks file handles when an IOException occurs during the transition from memory to file storage. The FileOutputStream created in the update() method is never closed if write() or flush() fails, leading to resource exhaustion.

Solution

Use proper exception handling with try-catch to ensure the FileOutputStream is closed if an exception occurs during the transition, while preserving it on success when it becomes the active output stream.

Changes

• Added proper resource management in FileBackedOutputStream.update() method
• FileOutputStream declared outside try block for proper cleanup scope
• Stream is closed only when an exception occurs (with suppressed exceptions)
• Exception handling preserves original IOException while cleaning up resources
• No changes to public API or behavior

Testing

# Run specific tests
./mvnw test -Dtest=FileBackedOutputStreamTest -pl guava-tests

# Run with coverage for the affected class
./mvnw test -Dtest=FileBackedOutputStream* -pl guava-tests

All existing tests pass. Added regression tests (testThresholdCrossing_ResourceManagement and testMultipleThresholdCrossings) to verify normal threshold-crossing behavior works correctly with the resource management improvements.

Fixes #5756

[cpovirk]

Would it makes sense to perform the cleanup in the existing catch (IOException e) block? That should be the error case that we need to worry about. (And if we're worried about other cases, we could generalize it to also catch RuntimeException.) That would also let us attach any exception from close as a suppressed exception on e.

[lmcrean]

thanks @cpovirk I've put cleanup in the catch block now:

        } catch (IOException e) {
        if (transfer != null) {
          try {
            transfer.close();
          } catch (IOException closeException) {
            e.addSuppressed(closeException);
          }
        }
        temp.delete();
        throw e;
      }

[lmcrean]

re. RuntimeException I get what you mean, may i suggest documenting as a new issue to keep the scope clean.

Once merged can add something like:

FileBackedOutputStream: extend resource leak fix to RuntimeException

Current fix addresses IOException resource leaks during memory-to-file transition. Should extend to catch RuntimeException (SecurityException, system errors) for complete resource cleanup.

Simple change: catch (IOException | RuntimeException e)

RuntimeException scenarios are less common than IOException cases.

[cpovirk]

Thanks, I'm actually happy to ignore the RuntimeException case here.

[cpovirk]

Is there a short summary of what the new tests cover beyond what the existing testThreshold tests cover? I'm sure I can work it out, but you may know offhand :)

[lmcrean]

The new tests are regression tests that were added alongside the resource management fix to verify normal operation works correctly.

• testThresholdCrossing_ResourceManagement() tests normal threshold crossing behavior through the memory-to-file transition
• testMultipleThresholdCrossings() tests normal operation across repeated threshold crossings

[cpovirk]

I think that threshold crossing in some sense is covered by testThreshold(), but it looks like that covers only the case in which we have written exactly the number of bytes that fit before and then write some new bytes that immediately trigger the use of the file. Your new test writes a smaller number of bytes and then writes a large number at once, enough to take us from "not at the threshold" to "over the threshold." So that seems like a new thing.

For "multiple threshold crossings," I don't think that's the phrasing I'd use, but your test is covering a write that occurs after the write that crosses the threshold. I don't think we cover that yet, except in the singleByte case, which doesn't take the same code path as you're covering.

Does that all sound right?

[lmcrean]

Yes, that all sounds right!

For testThresholdCrossing_ResourceManagement(): You've understood correctly. The existing testThreshold() writes exactly the number of bytes that fit (reaching the threshold), then immediately writes more bytes. My new test covers a different scenario: writing a smaller amount first (40 bytes), then writing a large amount at once (30 bytes) that takes us from "not at threshold" to "over the threshold."

For testMultipleThresholdCrossings(): This is indeed misleading phrasing. The test is actually covering writes that occur after the initial write that crosses the threshold. Once we've crossed the threshold and transitioned to file storage, this test verifies that continued writes to the file work correctly. As you noted, this scenario isn't covered except in the singleByte case, which takes a different code path.

I'll update the PR to:

1. Rename testMultipleThresholdCrossings() to testWriteAfterThresholdCrossing()
2. Clarify the JavaDoc for both tests to accurately describe what they're testing
3. Update the documentation to emphasize how testThresholdCrossing_ResourceManagement() differs from the existing testThreshold()

Thanks for the careful review and clarification!