Close the stream parse(Path) and parse(URL) open on construction failure

[rootvector2]

parse(Path) and parse(URL) allocate the input stream themselves and the CSVParser constructor throws before the caller gets a parser, so the stream the library opened leaked. The fix is in master now (f1bf8cd), so this PR is down to the better Path test suggested in review.

The temp file test only asserted the throw; deleting an open file succeeds on POSIX, so it passed without the fix. Files.newInputStream(path) dispatches through path.getFileSystem().provider(), so the test now mocks that chain and hands parse(Path) a close-recording stream, mirroring the URL test. It fails on master with the close in parse(IOSupplier) removed and passes as is, on any platform.

Full default mvn build is green.

• Read the contribution guidelines for this project.
• Read the ASF Generative Tooling Guidance if you use Artificial Intelligence (AI).
• I used AI to create any part of, or all of, this pull request. Which AI tool was used to create this pull request, and to what extent did it contribute?
• Run a successful build using the default Maven goal with mvn; that's mvn on the command line by itself.
• Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible, but it is a best practice.
• Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• Each commit in the pull request should have a meaningful subject line and body. Note that a maintainer may squash commits during the merge process.

[garydgregory]

Hello @rootvector2

This one feels wrong to me.

In git master, please find the new CSVParserTest.testTryWithResourcesParse*

• testTryWithResourcesParseInputStreamWhenHeaderIsInvalid
• testTryWithResourcesParseReaderWhenHeaderIsInvalid

It is the responsibility of the code that allocates a resource to close it.

If the the resource is managed with a try-with-resources block (see the new tests), the resource is closed for this use case. I switched the PR to draft for this reason.

Please let me know how you see it.

TY!

[rootvector2]

Agreed on caller-supplied readers; the new testTryWithResourcesParse* tests cover that case, so I dropped that part.

The case I was after is narrower: parse(Path) and parse(URL) allocate the stream themselves via Files.newInputStream(path) and url.openStream(). When createHeaders() throws, the caller never gets a parser back, so there is nothing to put in a try-with-resources block and the stream the library opened is unreachable. Applying the same rule, the method that allocated it now closes it on failure; caller-supplied readers and streams are untouched. Reworked the PR that way and rebased onto master. Your new tests still pass, and testParseUrlClosesInputStreamWhenHeaderIsInvalid fails without the change.

[garydgregory]

@rootvector2 On macOS, the test testParsePathClosesInputStreamWhenHeaderIsInvalid passes without changes to main, so it doesn't show anything to fix.

[garydgregory]

@rootvector2
I brought this in with a refactored implementation but a better test for Path would be good, or not, up to you ;)

[garydgregory]

@rootvector2 ping 🔔

[rootvector2]

Right, the temp file test showed nothing: deleting an open file succeeds on POSIX, so the only observable effect was the throw. Reworked the PR into just a better Path test on top of your refactoring. Files.newInputStream dispatches through path.getFileSystem().provider(), so mocking that chain lets the test hand parse(Path) a close-recording stream, same shape as the URL test. It fails on master with the close in parse(IOSupplier) removed and passes as is, on any platform. Full default mvn build is green. Thanks for bringing the fix in.