Continue deleting other files when some fail to delete in FileUtils by torusrxxx · Pull Request #1103 · hyphanet/fred

torusrxxx · 2025-08-29T14:47:11Z

secureDeleteAll should delete all files in a directory using secureDeleteAll.
Continue deleting other files when some of the files fail to delete in FileUtils.
Use distinct temp file names in tests.
tests should delete temp directories on exit.

…and use distinct temp file names in tests.

bertm

Nice catch!

bertm · 2025-08-29T17:53:32Z

test/freenet/client/filter/ContentFilterTest.java

-import java.io.InputStream;
-import java.io.OutputStream;
+
+import java.io.*;


Please don't collapse imports into wildcard imports.

bertm · 2025-08-29T17:57:13Z

test/freenet/client/filter/ContentFilterTest.java

        try (
            FileOutputStream fos = new FileOutputStream("output.utf16")
        ){
+            new File("output.utf16").deleteOnExit();


This and the below instances should really use a finally block instead of resorting to JVM shutdown hooks with deleteOnExit.

bertm · 2025-08-29T18:06:44Z

As a side note: really, these methods should be rewritten using Files.walkFileTree as in their current shape they will follow symbolic links into directories, which is a frightening idea when deleting!

None of that is introduced in this change, so feel free to not do anything about it in this PR.

Also, "secure deleting" is becoming increasingly meaningless in the modern days of copy-on-write filesystems and SSD drives (which are essentially copy-on-write hardware) ... but that's for another day 😉

Bombe · 2025-08-30T07:17:57Z

src/freenet/support/io/FileUtil.java

+			boolean success = true;
 			for(File subfile: wd.listFiles()) {
-				if(!removeAll(subfile)) return false;
+				if(!secureDeleteAll(subfile)){


Suggested change

if(!secureDeleteAll(subfile)){

success &= secureDeleteAll(subfile);

Bombe · 2025-08-30T07:18:08Z

src/freenet/support/io/FileUtil.java

+			boolean success = true;
 			for(File subfile: wd.listFiles()) {
-				if(!removeAll(subfile)) return false;
+				if(!removeAll(subfile)){


Bombe · 2025-08-30T07:19:29Z

test/freenet/client/async/ClientRequestSelectorTest.java


+    @After
+    public void tearDown() {
+        FileUtil.removeAll(dir);


While that is certainly a possibility, JUnit offers a TemporaryDirectory rule that takes care of this automatically; can that be used, maybe?

JUnit 5 does not support @Rule. I think we will need to migrate to JUnit 5 and therefore stop using @Rule.

I don’t see a pressing reason to migrate to JUnit 5, JUnit 4 certainly seems to work fine, so @Rule it is — unless it’s more of a hassle than it’s worth (which can happen if lots of objects get constructed before the actual test method is running). 😁

No, we still need to migrate to JUnit 5 simply because we will not be using JUnit 4 forever. The migration to vintage engine should start as soon as possible so that new tests don't need to migrate from JUnit 4 to JUnit 5.

Uhm, we are still on Java 8, I’m pretty confident that a lot of time will pass until we actually need to migrate to JUnit 5. But even if we do, migrating a TemporaryFolder from a @Rule to a @TempDir is almost trivial.

Bombe · 2025-08-30T07:21:05Z

test/freenet/crypt/EncryptedRandomAccessBucketTest.java


    @Before
    public void setUp() {
+        base = new File("tmp.encrypted-random-access-thing-test");


If a member variable is only assigned once and doesn’t depend on anything else and its initialization does not throw an exception, please make the member variable final and assign it in the declaration.

bertm · 2025-09-04T19:06:45Z

@torusrxxx kindly stop adding to the scope of this PR - if you want to make other improvements, please use a different branch and file a separate PR. This serves two purposes:

to prevent delaying the merge of this PR (since every time you add something, it has to be re-reviewed and there may be additional comments unrelated to the original scope, causing further delay)
to keep the changelog clean (make it easier to track what changed when and why)

The changes in SerializerTest, ShortBufferTest, and SparseBitmapTest are not at all related to FileUtils or even files in general, please drop those changes for this PR (and feel free to open another PR for them).

Bombe · 2025-09-06T08:26:12Z

test/freenet/client/async/ClientRequestSelectorTest.java

-        dir = new File("split-file-inserter-storage-test");
-        dir.mkdir();
+        tempFolder = new TemporaryFolder();
+        tempFolder.create();


I’m not sure we should use the method that is documented with “for testing purposes only. Do not use.”

This is exactly the situation I was referring to in my earlier comment about how TemporaryFolder can’t be used during constructor invocation.

torusrxxx · 2025-10-28T16:13:42Z

It looks like part of this pull request will be superseded by #1107. I will have to reopen a new PR.
Also, I haven’t started any meaningful coding with video filters. I have been busy IRL, and unfortunately, work on video filters will likely not be started until at least after Christmas.

ArneBab · 2025-11-23T23:14:22Z

It looks like part of this pull request will be superseded by #1107. I will have to reopen a new PR. Also, I haven’t started any meaningful coding with video filters. I have been busy IRL, and unfortunately, work on video filters will likely not be started until at least after Christmas.

Thank you for the info! And don’t worry: real life must take precedence, otherwise you have little chance to stay active on the long run. I just stumbled over an article that described that burnout is one of the prime reasons for FOSS devs to stop, so keeping the balance is vitally important.

Continue deleting other files when some fail to delete in FileUtils, …

bd76098

…and use distinct temp file names in tests.

bertm suggested changes Aug 29, 2025

View reviewed changes

Bombe requested changes Aug 30, 2025

View reviewed changes

Update tests and use distinct temp file names in tests

c966278

torusrxxx marked this pull request as ready for review August 30, 2025 14:18

torusrxxx added 2 commits September 3, 2025 17:23

Use JUnit 4 TemporaryFolder

aa59168

Fix javadoc

588917b

Bombe reviewed Sep 6, 2025

View reviewed changes

torusrxxx mentioned this pull request Nov 13, 2025

Continue deleting other files when some fail to delete in FileUtils #1112

Merged

	if(!secureDeleteAll(subfile)){
	success &= secureDeleteAll(subfile);

Conversation

torusrxxx commented Aug 29, 2025

Uh oh!

bertm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bertm commented Aug 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bombe Aug 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bertm commented Sep 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

torusrxxx commented Oct 28, 2025

Uh oh!

ArneBab commented Nov 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Bombe Aug 30, 2025 •

edited

Loading