From 1caa3c28796015e499f3b8750c4925ddaca6e028 Mon Sep 17 00:00:00 2001 From: w3stling Date: Sun, 15 Sep 2024 07:19:50 +0200 Subject: [PATCH 1/6] Close underlying resources when no timeout is used --- .../rssreader/AbstractRssReader.java | 50 ++++++++++++++----- .../RssReaderIntegrationTest.java | 33 ++++++++++-- 2 files changed, 67 insertions(+), 16 deletions(-) diff --git a/src/main/java/com/apptasticsoftware/rssreader/AbstractRssReader.java b/src/main/java/com/apptasticsoftware/rssreader/AbstractRssReader.java index 1a7b1c8..d45e399 100644 --- a/src/main/java/com/apptasticsoftware/rssreader/AbstractRssReader.java +++ b/src/main/java/com/apptasticsoftware/rssreader/AbstractRssReader.java @@ -36,6 +36,7 @@ import java.io.BufferedInputStream; import java.io.IOException; import java.io.InputStream; +import java.lang.ref.Cleaner; import java.net.URI; import java.net.http.HttpClient; import java.net.http.HttpRequest; @@ -69,6 +70,7 @@ public abstract class AbstractRssReader { private static final Logger LOGGER = Logger.getLogger("com.apptasticsoftware.rssreader"); private static final ScheduledExecutorService EXECUTOR = new ScheduledThreadPoolExecutor(1, new DaemonThreadFactory("RssReaderWorker")); + private static final Cleaner CLEANER = Cleaner.create(); private final HttpClient httpClient; private DateTimeParser dateTimeParser = new DateTime(); private String userAgent = ""; @@ -556,10 +558,36 @@ private void removeBadData(InputStream inputStream) { } catch (IOException ignore) { } } - class RssItemIterator implements Iterator { + private static class CleaningAction implements Runnable { + private final XMLStreamReader xmlStreamReader; + private final List resources; + + public CleaningAction(XMLStreamReader xmlStreamReader, AutoCloseable... resources) { + this.xmlStreamReader = xmlStreamReader; + this.resources = List.of(resources); + } + + @Override + public void run() { + try { + xmlStreamReader.close(); + } catch (XMLStreamException e) { + LOGGER.log(Level.WARNING, "Failed to close XML stream. ", e); + } + + for (AutoCloseable resource : resources) { + try { + resource.close(); + } catch (Exception e) { + LOGGER.log(Level.WARNING, "Failed to close resource. ", e); + } + } + } + } + + class RssItemIterator implements Iterator, AutoCloseable { private final StringBuilder textBuilder; private final Map childNodeTextBuilder; - private final InputStream is; private final Deque elementStack; private XMLStreamReader reader; private C channel; @@ -569,9 +597,9 @@ class RssItemIterator implements Iterator { private boolean isItemPart = false; private ScheduledFuture parseWatchdog; private final AtomicBoolean isClosed; + private Cleaner.Cleanable cleanable; public RssItemIterator(InputStream is) { - this.is = is; nextItem = null; textBuilder = new StringBuilder(); childNodeTextBuilder = new HashMap<>(); @@ -585,6 +613,7 @@ public RssItemIterator(InputStream is) { xmlInputFactory.setProperty(XMLInputFactory.IS_NAMESPACE_AWARE, false); reader = xmlInputFactory.createXMLStreamReader(is); + cleanable = CLEANER.register(this, new CleaningAction(reader, is)); if (!readTimeout.isZero()) { parseWatchdog = EXECUTOR.schedule(this::close, readTimeout.toMillis(), TimeUnit.MILLISECONDS); } @@ -595,16 +624,11 @@ public RssItemIterator(InputStream is) { } public void close() { - if (isClosed.compareAndSet(false,true)) { - try { - if (parseWatchdog != null) { - parseWatchdog.cancel(false); - } - reader.close(); - is.close(); - } catch (XMLStreamException | IOException e) { - LOGGER.log(Level.WARNING, "Failed to close XML stream. ", e); - } + if (isClosed.compareAndSet(false, true)) { + Optional.ofNullable(cleanable).ifPresent(Cleaner.Cleanable::clean); + cleanable = null; + Optional.ofNullable(parseWatchdog).ifPresent(sf -> sf.cancel(false)); + parseWatchdog = null; } } diff --git a/src/test/java/com/apptasticsoftware/integrationtest/RssReaderIntegrationTest.java b/src/test/java/com/apptasticsoftware/integrationtest/RssReaderIntegrationTest.java index bb9f646..bdf6a61 100644 --- a/src/test/java/com/apptasticsoftware/integrationtest/RssReaderIntegrationTest.java +++ b/src/test/java/com/apptasticsoftware/integrationtest/RssReaderIntegrationTest.java @@ -6,6 +6,7 @@ import javax.net.ssl.SSLContext; import java.io.*; +import java.lang.ref.Reference; import java.net.*; import java.net.http.HttpClient; import java.net.http.HttpRequest; @@ -492,11 +493,13 @@ private String getRssFeedAsString(String url) throws IOException, InterruptedExc .GET() .build(); - var client = HttpClient.newBuilder() + HttpResponse response; + try (var client = HttpClient.newBuilder() .followRedirects(HttpClient.Redirect.NORMAL) - .build(); + .build()) { - var response = client.send(req, HttpResponse.BodyHandlers.ofString()); + response = client.send(req, HttpResponse.BodyHandlers.ofString()); + } return response.body(); } @@ -605,6 +608,30 @@ void testCloseTwice() throws IOException { } } + @Test + void testCloseWithCleaner() { + var fileInputSteam = fromFile("atom-feed.xml"); + var stream = new RssReader().setReadTimeout(Duration.ZERO).read(fileInputSteam); + var iterator = stream.iterator(); + var item = iterator.next(); + assertNotNull(item); + + iterator = null; // Remove this reference to iterator + Reference.reachabilityFence(iterator); // Ensure data is not over-optimitically gc'd + stream = null; // Remove this reference to stream + Reference.reachabilityFence(stream); // Ensure data is not over-optimitically gc'd + + for (int retries = 10; retries > 0; retries--) { + System.gc(); + try { + Thread.sleep(10L); + } catch (InterruptedException ignore) { } + } + + IOException thrown = assertThrows(IOException.class, fileInputSteam::available); + assertEquals(thrown.getMessage(), "Stream closed"); + } + @SuppressWarnings("java:S5961") @Test void testAtomFeed() { From 606f939974efcf6ed59154672d48d919e5946c71 Mon Sep 17 00:00:00 2001 From: w3stling Date: Sun, 15 Sep 2024 07:48:23 +0200 Subject: [PATCH 2/6] Fix code smell --- .../rssreader/AbstractRssReader.java | 16 ++++++++++------ .../RssReaderIntegrationTest.java | 5 +++-- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/main/java/com/apptasticsoftware/rssreader/AbstractRssReader.java b/src/main/java/com/apptasticsoftware/rssreader/AbstractRssReader.java index d45e399..bc792e4 100644 --- a/src/main/java/com/apptasticsoftware/rssreader/AbstractRssReader.java +++ b/src/main/java/com/apptasticsoftware/rssreader/AbstractRssReader.java @@ -570,14 +570,18 @@ public CleaningAction(XMLStreamReader xmlStreamReader, AutoCloseable... resource @Override public void run() { try { - xmlStreamReader.close(); + if (xmlStreamReader != null) { + xmlStreamReader.close(); + } } catch (XMLStreamException e) { LOGGER.log(Level.WARNING, "Failed to close XML stream. ", e); } for (AutoCloseable resource : resources) { try { - resource.close(); + if (resource != null) { + resource.close(); + } } catch (Exception e) { LOGGER.log(Level.WARNING, "Failed to close resource. ", e); } @@ -625,10 +629,10 @@ public RssItemIterator(InputStream is) { public void close() { if (isClosed.compareAndSet(false, true)) { - Optional.ofNullable(cleanable).ifPresent(Cleaner.Cleanable::clean); - cleanable = null; - Optional.ofNullable(parseWatchdog).ifPresent(sf -> sf.cancel(false)); - parseWatchdog = null; + cleanable.clean(); + if (parseWatchdog != null) { + parseWatchdog.cancel(false); + } } } diff --git a/src/test/java/com/apptasticsoftware/integrationtest/RssReaderIntegrationTest.java b/src/test/java/com/apptasticsoftware/integrationtest/RssReaderIntegrationTest.java index bdf6a61..af190db 100644 --- a/src/test/java/com/apptasticsoftware/integrationtest/RssReaderIntegrationTest.java +++ b/src/test/java/com/apptasticsoftware/integrationtest/RssReaderIntegrationTest.java @@ -608,6 +608,7 @@ void testCloseTwice() throws IOException { } } + @SuppressWarnings("java:S2925") @Test void testCloseWithCleaner() { var fileInputSteam = fromFile("atom-feed.xml"); @@ -625,11 +626,11 @@ void testCloseWithCleaner() { System.gc(); try { Thread.sleep(10L); - } catch (InterruptedException ignore) { } + } catch (InterruptedException ignored) { /* ignore */ } } IOException thrown = assertThrows(IOException.class, fileInputSteam::available); - assertEquals(thrown.getMessage(), "Stream closed"); + assertEquals("Stream closed", thrown.getMessage()); } @SuppressWarnings("java:S5961") From 634ee8eb2f8c03d65d7c7b17eaa70a7b57d162e5 Mon Sep 17 00:00:00 2001 From: w3stling Date: Sun, 15 Sep 2024 08:40:37 +0200 Subject: [PATCH 3/6] Fix --- build.gradle | 4 ++++ .../rssreader/RssReaderTest.java | 17 ++++++++--------- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/build.gradle b/build.gradle index c9a9443..ba38ddc 100644 --- a/build.gradle +++ b/build.gradle @@ -50,6 +50,10 @@ compileJava { } } +compileTestJava { + options.compilerArgs = [ '-Xlint:unchecked' ] +} + jacoco { toolVersion = "0.8.9" } diff --git a/src/test/java/com/apptasticsoftware/rssreader/RssReaderTest.java b/src/test/java/com/apptasticsoftware/rssreader/RssReaderTest.java index 05c8332..2eeda6b 100644 --- a/src/test/java/com/apptasticsoftware/rssreader/RssReaderTest.java +++ b/src/test/java/com/apptasticsoftware/rssreader/RssReaderTest.java @@ -47,7 +47,7 @@ void badFormattedXml() throws IOException { "\n" + "\n"; - CompletableFuture> httpResponse = createMock(response); + var httpResponse = createMock(response); RssReader readerMock = Mockito.spy(RssReader.class); doReturn(httpResponse).when(readerMock).sendAsyncRequest(anyString()); @@ -78,7 +78,7 @@ void leadingCRCharacter() throws IOException { "\n" + "\n"; - CompletableFuture> httpResponse = createMock(response); + var httpResponse = createMock(response); RssReader readerMock = Mockito.spy(RssReader.class); doReturn(httpResponse).when(readerMock).sendAsyncRequest(anyString()); @@ -125,7 +125,7 @@ void leadingCRLDCharacters() throws IOException { "\n" + "\n"; - CompletableFuture> httpResponse = createMock(response); + var httpResponse = createMock(response); RssReader readerMock = Mockito.spy(RssReader.class); doReturn(httpResponse).when(readerMock).sendAsyncRequest(anyString()); @@ -172,7 +172,7 @@ void leadingWhitespace() throws IOException { "\n" + "\n"; - CompletableFuture> httpResponse = createMock(response); + var httpResponse = createMock(response); RssReader readerMock = Mockito.spy(RssReader.class); doReturn(httpResponse).when(readerMock).sendAsyncRequest(anyString()); @@ -219,7 +219,7 @@ void Cdata() throws IOException { "\n" + "\n"; - CompletableFuture> httpResponse = createMock(response); + var httpResponse = createMock(response); RssReader readerMock = Mockito.spy(RssReader.class); doReturn(httpResponse).when(readerMock).sendAsyncRequest(anyString()); @@ -248,7 +248,7 @@ void Cdata() throws IOException { void emptyResponse() throws IOException { String response = ""; - CompletableFuture> httpResponse = createMock(response); + var httpResponse = createMock(response); RssReader readerMock = Mockito.spy(RssReader.class); doReturn(httpResponse).when(readerMock).sendAsyncRequest(anyString()); @@ -514,9 +514,8 @@ void equalsContract() { } - private CompletableFuture> createMock(String response) { - HttpResponse httpResponse = mock(HttpResponse.class); - + private CompletableFuture createMock(String response) { + var httpResponse = mock(HttpResponse.class); InputStream responseStream = new ByteArrayInputStream(response.getBytes()); doReturn(responseStream).when(httpResponse).body(); From 6b56f0f12f8e612d3c9e6f20bc0371fced1af777 Mon Sep 17 00:00:00 2001 From: w3stling Date: Sun, 15 Sep 2024 08:44:18 +0200 Subject: [PATCH 4/6] Fix --- build.gradle | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/build.gradle b/build.gradle index ba38ddc..adc839d 100644 --- a/build.gradle +++ b/build.gradle @@ -44,16 +44,13 @@ compileJava { inputs.property('moduleName', moduleName) doFirst { options.compilerArgs = [ + '-Xlint:unchecked', '--module-path', classpath.asPath ] classpath = files() } } -compileTestJava { - options.compilerArgs = [ '-Xlint:unchecked' ] -} - jacoco { toolVersion = "0.8.9" } From 405e43708052f9817fd95742a135d4bc0c8ebd84 Mon Sep 17 00:00:00 2001 From: w3stling Date: Sun, 15 Sep 2024 08:44:41 +0200 Subject: [PATCH 5/6] Fix --- build.gradle | 1 - 1 file changed, 1 deletion(-) diff --git a/build.gradle b/build.gradle index adc839d..c9a9443 100644 --- a/build.gradle +++ b/build.gradle @@ -44,7 +44,6 @@ compileJava { inputs.property('moduleName', moduleName) doFirst { options.compilerArgs = [ - '-Xlint:unchecked', '--module-path', classpath.asPath ] classpath = files() From 45868ed779250ae8e3f424810e6221cca81fc570 Mon Sep 17 00:00:00 2001 From: w3stling Date: Sun, 15 Sep 2024 08:52:09 +0200 Subject: [PATCH 6/6] Fix --- .github/workflows/codeql-analysis.yml | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml index a2f5f04..80eec6a 100644 --- a/.github/workflows/codeql-analysis.yml +++ b/.github/workflows/codeql-analysis.yml @@ -43,8 +43,8 @@ jobs: # Autobuild attempts to build any compiled languages (C/C++, C#, or Java). # If this step fails, then you should remove it and run the build manually (see below) - - name: Autobuild - uses: github/codeql-action/autobuild@v3 + #- name: Autobuild + # uses: github/codeql-action/autobuild@v3 # ℹī¸ Command-line programs to run using the OS shell. # 📚 https://git.io/JvXDl @@ -53,9 +53,7 @@ jobs: # and modify them (or add more) to build your code if your project # uses a compiled language - #- run: | - # make bootstrap - # make release + - run: ./gradlew clean build -x test - name: Perform CodeQL analysis uses: github/codeql-action/analyze@v3