You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "rootvector2 (via GitHub)" <gi...@apache.org> on 2026/08/01 10:30:22 UTC
[PR] close the input when createHeaders throws in the CSVParser constructor [commons-csv]
rootvector2 opened a new pull request, #630:
URL: https://github.com/apache/commons-csv/pull/630
`createHeaders()` runs inside the `CSVParser` constructor, after the lexer already owns the reader returned by `builder.getReader()`, so a duplicate or missing header name throws before the caller receives a `Closeable` and nothing is left to close the input with. For `parse(Path)`, `parse(File)`, `parse(URL)` and the `Path`/`File`/`URI` builder forms the library opened that stream itself, so the descriptor or connection stays open for the lifetime of the JVM. Found while checking what happens to the handle when a header fails validation.
The constructor is the layer that took ownership and every entry point funnels its reader through it, so closing there covers all of them; a close failure is attached as suppressed so the original error still surfaces.
- [x] Read the [contribution guidelines](CONTRIBUTING.md) for this project.
- [ ] Read the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) 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?
- [x] Run a successful build using the default [Maven](https://maven.apache.org/) goal with `mvn`; that's `mvn` on the command line by itself.
- [x] 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.
- [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [x] 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] close the stream parse(Path) and parse(URL) open on construction failure [commons-csv]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #630:
URL: https://github.com/apache/commons-csv/pull/630#issuecomment-5196096335
@rootvector2
I brought this in with a refactored implementation but a better test for Path would be good, or not, up to you ;)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Close the stream parse(Path) and parse(URL) open on construction failure [commons-csv]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory merged PR #630:
URL: https://github.com/apache/commons-csv/pull/630
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] close the stream parse(Path) and parse(URL) open on construction failure [commons-csv]
Posted by "rootvector2 (via GitHub)" <gi...@apache.org>.
rootvector2 commented on PR #630:
URL: https://github.com/apache/commons-csv/pull/630#issuecomment-5239330121
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] close the input when createHeaders throws in the CSVParser constructor [commons-csv]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #630:
URL: https://github.com/apache/commons-csv/pull/630#issuecomment-5151231875
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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] close the stream parse(Path) and parse(URL) open on construction failure [commons-csv]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #630:
URL: https://github.com/apache/commons-csv/pull/630#issuecomment-5226344295
@rootvector2 ping 🔔
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] close the stream parse(Path) and parse(URL) open on construction failure [commons-csv]
Posted by "rootvector2 (via GitHub)" <gi...@apache.org>.
rootvector2 commented on PR #630:
URL: https://github.com/apache/commons-csv/pull/630#issuecomment-5194538489
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org