fix(xml): close DTD resolver resources by w3lld1 · Pull Request #3334 · testng-team/testng

w3lld1 · 2026-08-08T19:09:03Z

Fixes #3316.

Summary

I buffer resolved DTD content before returning it to the SAX parser, so resolver-owned streams are closed immediately.
I apply 10-second connect and read timeouts to external DTD connections.
I disconnect HTTP connections after reading them, including the original connection on a manual redirect.
I added regression coverage for buffering/closing external DTDs and configuring both timeouts.

Validation

./gradlew :testng-core:test --tests test.xml.TestNGContentHandlerTest --rerun-tasks
./gradlew build
./gradlew autostyleCheck
./gradlew rewriteDryRun -PfailOnRewriteDryRun=true
git diff --check

Did you remember to?

Add test case(s)
Update CHANGES.txt
Auto applied styling via ./gradlew autostyleApply

Summary by CodeRabbit

Bug Fixes
- Improved external DTD handling by reliably releasing network and stream resources.
- Added 10-second connection and read timeouts for remote DTDs.
- Safely follows no more than one HTTP redirect.
- Improved support for non-HTTP DTD sources.
- Preserves DTD content after the original source or stream is closed.

coderabbitai · 2026-08-08T19:09:19Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 10c6c043-d2ff-4576-8c41-19f8b43609c0

📥 Commits

Reviewing files that changed from the base of the PR and between bd6175b and eab30ce.

📒 Files selected for processing (2)

testng-core/src/main/java/org/testng/xml/TestNGContentHandler.java
testng-core/src/test/java/org/testng/xml/TestNGContentHandlerTestSupport.java

🚧 Files skipped from review as they are similar to previous changes (2)

testng-core/src/test/java/org/testng/xml/TestNGContentHandlerTestSupport.java
testng-core/src/main/java/org/testng/xml/TestNGContentHandler.java

📝 Walkthrough

Walkthrough

TestNG 7.13.0 updates DTD resolution to bound URL waits, close resources, handle one redirect, and buffer DTD content. Tests cover timeout configuration, resource cleanup, buffered content, and redirect handling.

Changes

DTD resolver resource handling

Layer / File(s)	Summary
Configure connections and buffer DTD content `testng-core/src/main/java/org/testng/xml/TestNGContentHandler.java`	The resolver applies 10-second connection and read timeouts, handles one HTTP redirect, closes streams, disconnects HTTP connections, and returns buffered DTD content through `InputSource`.
Validate resolver behavior `testng-core/src/test/java/org/testng/xml/TestNGContentHandlerTestSupport.java`, `testng-core/src/test/java/test/xml/TestNGContentHandlerTest.java`, `CHANGES.txt`	Tests verify timeout configuration, buffered content after source closure, and one-redirect handling. The change log records the fix.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

testng-team/testng#3325: Both PRs modify TestNGContentHandler, including DTD resolution and parsing behavior.

Suggested reviewers: krmahadevan

Sequence Diagram(s)

sequenceDiagram
  participant XMLParser
  participant TestNGContentHandler
  participant DTDServer
  XMLParser->>TestNGContentHandler: Resolve external DTD
  TestNGContentHandler->>DTDServer: Open connection with timeouts
  DTDServer-->>TestNGContentHandler: DTD content or one redirect
  TestNGContentHandler-->>XMLParser: Buffered InputSource

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the primary change: closing DTD resolver resources.
Linked Issues check	✅ Passed	The changes address issue `#3316` by buffering DTDs, closing streams and connections, configuring timeouts, and limiting redirects.
Out of Scope Changes check	✅ Passed	The implementation, regression tests, test support, and changelog update directly support the linked issue objectives.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@testng-core/src/main/java/org/testng/xml/TestNGContentHandler.java`:
- Around line 76-80: Update the normal known-DTD path in loadDtdUsingClassLoader
to read the returned classpath stream into a byte array inside
try-with-resources, close it, and construct InputSource from the buffered bytes
rather than passing the stream directly to SAX.
- Around line 92-105: In the URL-reading flow around httpConnection and
getResponseCode(), disable HttpURLConnection’s automatic redirect handling
before reading the response code by configuring the connection instance.
Preserve the existing followRedirect status checks and recursive one-hop
handling, and add coverage proving a two-redirect chain follows only the first
redirect.

In `@testng-core/src/test/java/test/xml/TestNGContentHandlerTest.java`:
- Around line 61-69: Update the test around TestNGContentHandler and the
temporary dtd path so cleanup is unconditional: wrap handler.resolveEntity and
its assertions in a try/finally and call Files.deleteIfExists(dtd) from finally,
removing the current success-path Files.delete(dtd) call.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: bce5ec43-e068-4309-b128-31c20a5e29a1

📥 Commits

Reviewing files that changed from the base of the PR and between 90fb9b9 and 7ef7591.

📒 Files selected for processing (4)

CHANGES.txt
testng-core/src/main/java/org/testng/xml/TestNGContentHandler.java
testng-core/src/test/java/org/testng/xml/TestNGContentHandlerTestSupport.java
testng-core/src/test/java/test/xml/TestNGContentHandlerTest.java

w3lld1 · 2026-08-08T21:23:55Z

Thanks — addressed all three findings in bd7e9498:

the standard classpath DTD path now buffers and closes its stream;
HTTP redirects are handled explicitly, with regression coverage confirming that only one redirect is followed;
temporary-file cleanup now runs unconditionally.

Validation: focused TestNGContentHandlerTest (5 tests), autostyleCheck, and git diff --check pass.

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@testng-core/src/test/java/test/xml/TestNGContentHandlerTest.java`:
- Around line 63-78: Update resolverBuffersAndClosesExternalDtd to use an
instrumented URLConnection input stream that records closure, and verify the
source stream is closed before resolveEntity returns. Replace the temporary-file
deletion check with an assertion on that closure state, then separately read the
returned byte stream to confirm the buffered content remains available without
relying on the test closing the handler-owned source stream.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 4d54f5b8-b0bb-4c43-854c-dd85881d7b1f

📥 Commits

Reviewing files that changed from the base of the PR and between 7ef7591 and bd7e949.

📒 Files selected for processing (3)

testng-core/src/main/java/org/testng/xml/TestNGContentHandler.java
testng-core/src/test/java/org/testng/xml/TestNGContentHandlerTestSupport.java
testng-core/src/test/java/test/xml/TestNGContentHandlerTest.java

🚧 Files skipped from review as they are similar to previous changes (2)

testng-core/src/test/java/org/testng/xml/TestNGContentHandlerTestSupport.java
testng-core/src/main/java/org/testng/xml/TestNGContentHandler.java

w3lld1 · 2026-08-08T21:35:46Z

Thanks — addressed the follow-up in bd6175b4. The regression test now uses an instrumented source stream, verifies that it is closed before the resolver helper returns, and separately confirms that the returned buffered stream remains readable.

The focused TestNGContentHandlerTest (5 tests), autostyleCheck, and git diff --check pass.

juherr · 2026-08-09T19:19:24Z

+          return readUrlAsInputSource(url);
+        }
+        try (InputStream input = stream) {
+          return new InputSource(new ByteArrayInputStream(input.readAllBytes()));


add a comment to explain why bytes are read instead of new InputSource(input)

same for the second usage

juherr · 2026-08-09T19:22:25Z

+    }
+  }
+
+  static InputSource readUrlAsInputSource(URL url) throws IOException {


only used by tests, move it into the test workspace

fix(xml): close DTD resolver resources

7ef7591

w3lld1 requested review from juherr and krmahadevan as code owners August 8, 2026 19:09

coderabbitai Bot reviewed Aug 8, 2026

View reviewed changes

Comment thread testng-core/src/main/java/org/testng/xml/TestNGContentHandler.java

Comment thread testng-core/src/main/java/org/testng/xml/TestNGContentHandler.java

Comment thread testng-core/src/test/java/test/xml/TestNGContentHandlerTest.java Outdated

fix(xml): address resolver review feedback

bd7e949

coderabbitai Bot reviewed Aug 8, 2026

View reviewed changes

Comment thread testng-core/src/test/java/test/xml/TestNGContentHandlerTest.java

test(xml): verify resolver source closure

bd6175b

juherr requested changes Aug 9, 2026

View reviewed changes

fix(xml): address maintainer review

eab30ce

Navigation Menu

Uh oh!

fix(xml): close DTD resolver resources - #3334

fix(xml): close DTD resolver resources#3334
w3lld1 wants to merge 4 commits into
testng-team:masterfrom
w3lld1:fix-testng-content-handler-resources

w3lld1 commented Aug 8, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Aug 8, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

w3lld1 commented Aug 8, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

w3lld1 commented Aug 8, 2026

Uh oh!

juherr Aug 9, 2026

Uh oh!

juherr Aug 9, 2026

Uh oh!

juherr Aug 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

w3lld1 commented Aug 8, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Validation

Did you remember to?

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Aug 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

w3lld1 commented Aug 8, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

w3lld1 commented Aug 8, 2026

Uh oh!

juherr Aug 9, 2026

Choose a reason for hiding this comment

Uh oh!

juherr Aug 9, 2026

Choose a reason for hiding this comment

Uh oh!

juherr Aug 9, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

w3lld1 commented Aug 8, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Aug 8, 2026 •

edited

Loading