[IO-782] SequenceReader should close readers when its close method is called by digiovinazzo · Pull Request #391 · apache/commons-io

digiovinazzo · 2022-10-05T17:59:18Z

Pull request to fix https://issues.apache.org/jira/browse/IO-782

codecov-commenter · 2022-10-05T19:13:53Z

Codecov Report

Merging #391 (17f7689) into master (c1d9f7f) will decrease coverage by 0.09%.
The diff coverage is 71.42%.

@@             Coverage Diff              @@
##             master     #391      +/-   ##
============================================
- Coverage     86.18%   86.08%   -0.10%     
+ Complexity     3214     3213       -1     
============================================
  Files           215      215              
  Lines          7490     7503      +13     
  Branches        906      908       +2     
============================================
+ Hits           6455     6459       +4     
- Misses          791      797       +6     
- Partials        244      247       +3

Impacted Files	Coverage Δ
...va/org/apache/commons/io/input/SequenceReader.java	`89.36% <71.42%> (-7.70%)`	⬇️
.../apache/commons/io/input/ReadAheadInputStream.java	`68.53% <0.00%> (-3.38%)`	⬇️
.../main/java/org/apache/commons/io/input/Tailer.java	`86.06% <0.00%> (+0.49%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

src/main/java/org/apache/commons/io/input/SequenceReader.java

garydgregory

I don't think I agree with the premise of this PR: the class did not allocate the resources, so it has no business closing them, it does not own these resources.
What does the community think?

src/main/java/org/apache/commons/io/input/SequenceReader.java

digiovinazzo · 2022-10-07T16:56:53Z

I checked for example BufferedReader and it does close the Reader passed in the constructor.
Moreover even the old java.io.SequenceInputStream closes its InputStreams.

garydgregory · 2022-10-07T18:47:45Z

@digiovinazzo
Thank you for the clarification, this PR seems to make sense then :-)

garydgregory · 2022-10-07T18:50:00Z

src/test/java/org/apache/commons/io/input/SequenceReaderTest.java

+            assertEquals('A', sequenceReader.read());
+            assertEquals(EOF, sequenceReader.read());
+        } catch (IOException e) {
+            fail("No IOException expected");


Why not just let the exception percolate to fail the test?

That's another option, it was just to have a meaningful message on fail

Simpler is better:

try (SequenceReader sequenceReader = new SequenceReader(reader1, emptyReader)) { assertEquals('A', sequenceReader.read()); assertEquals(EOF, sequenceReader.read()); } finally { assertTrue(closeEmptyReader.get()); assertTrue(closeReader1.get()); }

src/main/java/org/apache/commons/io/input/SequenceReader.java

garydgregory · 2022-10-09T15:13:12Z

src/main/java/org/apache/commons/io/input/SequenceReader.java

+            try {
+                reader.close();
+            } catch (IOException e) {
+                ioExceptionList.add(e);


I'm not sure about the semantics here, which are completely different from SequenceInputStream. For good or bad, I think it would be better for the closing logic of this class to match SequenceInputStream.

garydgregory · 2022-10-09T15:20:29Z

src/main/java/org/apache/commons/io/input/SequenceReader.java


    private Reader reader;
    private Iterator<? extends Reader> readers;
+    private Iterable<? extends Reader> readersIterable;


No need to change the name, it makes the PR noisier.

garydgregory · 2022-10-09T15:21:00Z

src/test/java/org/apache/commons/io/input/SequenceReaderTest.java

+        final AtomicBoolean closeEmptyReader = new AtomicBoolean(false);
+        final Reader emptyReader = new Reader() {
+            @Override
+            public int read(char[] cbuf, int off, int len) throws IOException {


Use final where you can.

garydgregory · 2022-10-09T15:21:40Z

src/test/java/org/apache/commons/io/input/SequenceReaderTest.java

+
+        final AtomicBoolean closeReader1 = new AtomicBoolean(false);
+        final Reader reader1 = new Reader() {
+            private final char[] content = new char[]{'A'};


Use abbreviated notation, IOW ... = {'A'};

garydgregory · 2022-10-09T15:21:44Z

src/test/java/org/apache/commons/io/input/SequenceReaderTest.java

+            private int position = 0;
+
+            @Override
+            public int read(char[] cbuf, int off, int len) throws IOException {


Use final where you can.

garydgregory · 2022-10-09T15:22:49Z

src/test/java/org/apache/commons/io/input/SequenceReaderTest.java

+            }
+        };
+
+        final AtomicBoolean closeReader1 = new AtomicBoolean(false);


No need to initialize an AtomicBoolean to false, that's the default, IOW -> new AtomicBoolean();

garydgregory · 2022-10-09T15:26:55Z

src/test/java/org/apache/commons/io/input/SequenceReaderTest.java

+            assertEquals('A', sequenceReader.read());
+            assertEquals(EOF, sequenceReader.read());
+        } catch (IOException e) {
+            fail("No IOException expected");


Simpler is better:

try (SequenceReader sequenceReader = new SequenceReader(reader1, emptyReader)) { assertEquals('A', sequenceReader.read()); assertEquals(EOF, sequenceReader.read()); } finally { assertTrue(closeEmptyReader.get()); assertTrue(closeReader1.get()); }

garydgregory · 2022-10-09T17:26:32Z

The issue is now resolved in git master by making the implementation closer to what SequenceInputStream does. Please verify your use case and close. TY!

called #391.

garydgregory · 2022-10-13T14:20:23Z

Closing per my previous comment and no feedback in 4 days.

digiovinazzo · 2022-10-17T17:18:16Z

I was offline last week, @garydgregory thanks for taking care of this!

IO-782 Separate tests for try-with-resource and explicit close()

be2ff22

garydgregory requested changes Oct 5, 2022

View reviewed changes

src/main/java/org/apache/commons/io/input/SequenceReader.java Outdated Show resolved Hide resolved

digiovinazzo force-pushed the IO-782-SequenceReader-should-close-readers-when-its-close-method-is-called branch 2 times, most recently from 6e357a9 to bbd7a98 Compare October 5, 2022 20:06

garydgregory requested changes Oct 6, 2022

View reviewed changes

src/main/java/org/apache/commons/io/input/SequenceReader.java Outdated Show resolved Hide resolved

digiovinazzo force-pushed the IO-782-SequenceReader-should-close-readers-when-its-close-method-is-called branch from bbd7a98 to 22d2bdc Compare October 7, 2022 16:54

garydgregory reviewed Oct 7, 2022

View reviewed changes

src/main/java/org/apache/commons/io/input/SequenceReader.java Outdated Show resolved Hide resolved

IO-782 Close all readers when closing the SequenceReader with unit test

33da613

digiovinazzo force-pushed the IO-782-SequenceReader-should-close-readers-when-its-close-method-is-called branch from 22d2bdc to 33da613 Compare October 7, 2022 20:09

garydgregory changed the title ~~IO-782 sequence reader should close readers when its close method is called~~ [IO-782] SequenceReader should close readers when its close method is called Oct 9, 2022

garydgregory reviewed Oct 9, 2022

View reviewed changes

garydgregory requested changes Oct 9, 2022

View reviewed changes

asfgit pushed a commit that referenced this pull request Oct 9, 2022

[IO-782] SequenceReader should close readers when its close method is

7b13b4d

called #391.

garydgregory closed this Oct 13, 2022

Conversation

digiovinazzo commented Oct 5, 2022

Uh oh!

codecov-commenter commented Oct 5, 2022

Codecov Report

Uh oh!

Uh oh!

garydgregory left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

digiovinazzo commented Oct 7, 2022

Uh oh!

garydgregory commented Oct 7, 2022

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!

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!

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!

garydgregory commented Oct 9, 2022

Uh oh!

garydgregory commented Oct 13, 2022

Uh oh!

digiovinazzo commented Oct 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants