Close underlying resources when no timeout is used

w3stling commented 2 months ago

User description

This PR fixes a special case where the underlying resource was not closed when using zero read timeout and reading the feed using an iterator or a spliterator.

Example:

var stream = new RssReader().setReadTimeout(Duration.ZERO).read(URL);
var iterator = stream.iterator();
var item = iterator.next();

PR Type

Bug fix, Enhancement, Tests

Description

Implemented a Cleaner in AbstractRssReader to ensure underlying resources are properly closed when no timeout is used.
Added a new CleaningAction class to handle the closing of XMLStreamReader and other resources.
Modified RssItemIterator to implement AutoCloseable and utilize Cleaner for resource management.
Enhanced the integration tests to include a test case for verifying resource cleanup using Cleaner.
Updated existing tests to use try-with-resources for better resource management.

Changes walkthrough 📝

Relevant files

Enhancement

AbstractRssReader.java

Implement resource cleanup using Cleaner in AbstractRssReader

src/main/java/com/apptasticsoftware/rssreader/AbstractRssReader.java

Introduced a Cleaner to manage resource cleanup.

Added CleaningAction class to handle resource closing.

Modified RssItemIterator to implement AutoCloseable.

Updated resource management logic to ensure proper closure.

+37/-13

Tests

RssReaderIntegrationTest.java

Add tests for resource cleanup and modify existing tests

src/test/java/com/apptasticsoftware/integrationtest/RssReaderIntegrationTest.java

Added a test to verify resource cleanup using Cleaner.

Modified existing test to use try-with-resources for HttpClient.

+30/-3

💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

github-actions[bot] commented 2 months ago

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Key issues to review

Resource Management
The `CleaningAction` class attempts to close resources, but it logs a warning without rethrowing the exception or handling it further. Consider rethrowing exceptions or implementing a more robust error handling strategy to ensure that resource leaks are properly addressed. Concurrency Concern
The `RssItemIterator` class uses `AtomicBoolean` for `isClosed` state management, but there's a potential race condition between checking and setting this state in the `close` method. Consider using a lock or synchronizing the method to ensure thread safety.

github-actions[bot] commented 2 months ago

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	Add null checks before attempting to close resources to prevent NullPointerException ___ Consider handling the potential `NullPointerException` that could occur if `xmlStreamReader` or any of the `resources` in the `CleaningAction` are `null` when attempting to close them. [src/main/java/com/apptasticsoftware/rssreader/AbstractRssReader.java [573-583]](https://github.com/w3stling/rssreader/pull/178/files#diff-5a097d88f39fdbd552c3310d7b5e1c547981bb185595f89a39dfa2d43982502cR573-R583) ```diff -try { - xmlStreamReader.close(); -} catch (XMLStreamException e) { - LOGGER.log(Level.WARNING, "Failed to close XML stream. ", e); +if (xmlStreamReader != null) { + 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); + if (resource != null) { + try { + resource.close(); + } catch (Exception e) { + LOGGER.log(Level.WARNING, "Failed to close resource. ", e); + } } } ``` Suggestion importance[1-10]: 8 Why: This suggestion addresses a potential bug by adding null checks before closing resources, which prevents NullPointerExceptions and ensures more robust error handling.	8
Best practice	Ensure proper cancellation of scheduled tasks before nullifying to prevent leaks ___ Ensure that the `ScheduledFuture parseWatchdog` is cancelled before setting it to `null` to avoid potential memory leaks or unexpected behavior. [src/main/java/com/apptasticsoftware/rssreader/AbstractRssReader.java [630-631]](https://github.com/w3stling/rssreader/pull/178/files#diff-5a097d88f39fdbd552c3310d7b5e1c547981bb185595f89a39dfa2d43982502cR630-R631) ```diff -Optional.ofNullable(parseWatchdog).ifPresent(sf -> sf.cancel(false)); -parseWatchdog = null; +if (parseWatchdog != null) { + parseWatchdog.cancel(false); + parseWatchdog = null; +} ``` Suggestion importance[1-10]: 7 Why: The suggestion improves code reliability by ensuring that scheduled tasks are properly cancelled before being set to null, which can help prevent memory leaks or unexpected behavior.	7
Best practice	Use PhantomReference and ReferenceQueue for more reliable resource cleanup checking in tests ___ Replace the use of `System.gc()` and `Thread.sleep()` in tests with a more reliable method for ensuring that resources are cleaned up, such as using `PhantomReference` and `ReferenceQueue`. [src/main/java/com/apptasticsoftware/rssreader/AbstractRssReader.java [625-628]](https://github.com/w3stling/rssreader/pull/178/files#diff-5a097d88f39fdbd552c3310d7b5e1c547981bb185595f89a39dfa2d43982502cR625-R628) ```diff -for (int retries = 10; retries > 0; retries--) { +// Example using PhantomReference and ReferenceQueue +ReferenceQueue queue = new ReferenceQueue<>(); +PhantomReference ref = new PhantomReference<>(item, queue); +item = null; +while (queue.poll() == null) { System.gc(); - try { - Thread.sleep(10L); - } catch (InterruptedException ignore) { } + Thread.sleep(10L); } ``` Suggestion importance[1-10]: 5 Why: This suggestion promotes a best practice by recommending a more reliable method for resource cleanup in tests, though the current approach may still be functional for its intended purpose.	5
Enhancement	Use more specific exceptions for clarity and better error handling ___ Consider using a more specific exception type or a custom exception instead of the generic `Exception` in the resource-closing loop to provide more clarity on what exceptions are expected. [src/main/java/com/apptasticsoftware/rssreader/AbstractRssReader.java [579-583]](https://github.com/w3stling/rssreader/pull/178/files#diff-5a097d88f39fdbd552c3310d7b5e1c547981bb185595f89a39dfa2d43982502cR579-R583) ```diff for (AutoCloseable resource : resources) { try { resource.close(); - } catch (Exception e) { + } catch (IOException \| XMLStreamException e) { LOGGER.log(Level.WARNING, "Failed to close resource. ", e); } } ``` Suggestion importance[1-10]: 6 Why: Using more specific exceptions enhances code clarity and improves error handling by making it clear what types of exceptions are expected during resource closing.	6