Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -171,4 +171,83 @@ private static boolean isAndroid() {
private static boolean isWindows() {
return OS_NAME.value().startsWith("Windows");
}

/**
* Test that verifies the resource leak fix for Issue #5756.
* This test ensures that when switching from memory to file storage,
* the FileOutputStream is properly managed even if an exception occurs.
*
* Note: Direct testing of the IOException scenario during write/flush is
* challenging without mocking. This test verifies that normal operation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Contributor Author

@lmcrean lmcrean Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

* still works correctly with the fix in place.
*/
public void testThresholdCrossing_ResourceManagement() throws Exception {
// Test data that will cross the threshold
int threshold = 50;
byte[] beforeThreshold = newPreFilledByteArray(40);
byte[] afterThreshold = newPreFilledByteArray(30);

FileBackedOutputStream out = new FileBackedOutputStream(threshold);
ByteSource source = out.asByteSource();

// Write data that doesn't cross threshold
out.write(beforeThreshold);
assertNull(out.getFile());

// Write data that crosses threshold - this exercises the fixed code path
if (!JAVA_IO_TMPDIR.value().equals("/sdcard")) {
out.write(afterThreshold);
File file = out.getFile();
assertNotNull(file);
assertTrue(file.exists());

// Verify all data was written correctly
byte[] expected = new byte[70];
System.arraycopy(beforeThreshold, 0, expected, 0, 40);
System.arraycopy(afterThreshold, 0, expected, 40, 30);
assertTrue(Arrays.equals(expected, source.read()));

// Clean up
out.close();
out.reset();
assertFalse(file.exists());
}
}

/**
* Test that verifies multiple threshold crossings work correctly
* with the resource leak fix in place.
*/
public void testMultipleThresholdCrossings() throws Exception {
// Use a small threshold to force multiple file operations
int threshold = 10;
FileBackedOutputStream out = new FileBackedOutputStream(threshold);
ByteSource source = out.asByteSource();

// Write data in chunks that will cause threshold crossing
byte[] chunk1 = newPreFilledByteArray(8); // Below threshold
byte[] chunk2 = newPreFilledByteArray(5); // Crosses threshold
byte[] chunk3 = newPreFilledByteArray(20); // More data to file

out.write(chunk1);
assertNull(out.getFile());

if (!JAVA_IO_TMPDIR.value().equals("/sdcard")) {
out.write(chunk2);
File file = out.getFile();
assertNotNull(file);

out.write(chunk3);

// Verify all data is correct
byte[] expected = new byte[33];
System.arraycopy(chunk1, 0, expected, 0, 8);
System.arraycopy(chunk2, 0, expected, 8, 5);
System.arraycopy(chunk3, 0, expected, 13, 20);
assertTrue(Arrays.equals(expected, source.read()));

out.close();
out.reset();
}
}
}
11 changes: 10 additions & 1 deletion guava/src/com/google/common/io/FileBackedOutputStream.java
Original file line number Diff line number Diff line change
Expand Up @@ -238,13 +238,22 @@ private void update(int len) throws IOException {
// this is insurance.
temp.deleteOnExit();
}
// Create and populate the file, ensuring proper resource management
FileOutputStream transfer = null;
try {
FileOutputStream transfer = new FileOutputStream(temp);
transfer = new FileOutputStream(temp);
transfer.write(memory.getBuffer(), 0, memory.getCount());
transfer.flush();
// We've successfully transferred the data; switch to writing to file
out = transfer;
} catch (IOException e) {
if (transfer != null) {
try {
transfer.close();
} catch (IOException closeException) {
e.addSuppressed(closeException);
}
}
temp.delete();
throw e;
}
Expand Down