ppkarwasz commented on PR #789: URL: https://github.com/apache/commons-io/pull/789#issuecomment-3343623768
Hi @garydgregory, > Let's remember that the end user for this code is a Builder. There are two use cases related to resource management for a Builder: > > * A client allocates a `Closeable` (or `AutoCloseable`) resource, creates a Builder, and gives the Builder that resource by calling a setter method. No matter what happens next, the client is responsible for releasing the resource (`Closeable.close()`). In this case, the origin wraps but doesn't own the closeable resource. There is no "transfer of ownership". > > * A client creates a Builder and gives it a non-`Closeable` object, like a `File` or a `Path`. The client then calls the Builder's factory method (`get()`), and that call returns a `Closeable` or a resource that requires releasing in some other way. No matter what happens next, the client is responsible for releasing that resource. In this case, the origin doesn't wrap a closeable resource. In the first case, I do see a kind of ownership transfer: * From the user to the builder when the user calls the setter. * From the builder to the resource created by `get()`, which then assumes control of the original resource. * Finally, to the caller of `get()`, who owns the returned resource but is no longer responsible for the original one, much like how constructing a `new InputStreamReader(in)` shifts responsibility from the caller to the reader, which will close the underlying stream. My main motivation for this PR is that even `AbstractOriginTest` unit tests show confusion around resource ownership. Many tests never close the resources created by `newOriginRo` or `newOriginRw`. This only became visible when I changed `newOriginRw` to use a temporary folder per test: since JUnit deletes the folder afterward, unclosed resources triggered exceptions on Windows. I’m not strongly attached to making `AbstractOrigin` itself `Closeable`, but I do think it’s important to signal to `AbstractStreamBuilder` implementors that resources returned by the getters are **not** close-shielded: closing them will also close the original resource. If that behavior is undesirable, it’s up to the implementation to wrap the returned resource in a close-shield, as `BoundedInputStream.Builder` already does. I noticed that in ec6d80a1f76e750702b25b2a6164617801460404 you added Javadoc to specific `AbstractOrigin` subclasses to indicate which methods return the origin directly. That’s useful, but since users generally don’t code against concrete subclasses, the Javadoc on `AbstractOrigin#getInputStream()` and similar methods should also be improved to define rules that apply consistently across **all** implementations. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
