Close underlying resources when no timeout is used

[w3stling]

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]

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]

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] -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] -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
	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] -for (int retries = 10; retries > 0; retries--) { +// Example using PhantomReference and ReferenceQueue +ReferenceQueue<Item> queue = new ReferenceQueue<>(); +PhantomReference<Item> 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] 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

[github-actions]

Test Results

 10 files   10 suites   32s ⏱️
274 tests 272 ✅ 2 💤 0 ❌
282 runs  280 ✅ 2 💤 0 ❌

Results for commit 45868ed.

♻️ This comment has been updated with latest results.

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
76.9% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud