When to close an injected Closeable resource?

[Andree Surya]

Hi there,

I have the following Guice binding in my application.

bind(Reader.class)
    .annotatedWith(DictionarySourceReader.class)
    .toProvider(DictionarySourceProvider.class);

DictionarySourceProvider will provide a FileReader object, which will be injected to a DictionarySourceParser.

public class DictionarySourceParser {

    private Reader sourceReader;

    @Inject
    DictionarySourceParser(@DictionarySourceReader Reader sourceReader) {
        this.sourceReader = sourceReader;
    }

    public List<DictionaryEntry> parse() {

        // Parsing entries from the reader ....

        return entries;
    }
}

I have two questions:

When DictionarySourceParser is done with the Reader, who has the responsibility to close the reader? Should the parser be responsible?
Is it a commonplace to inject a resource that should be cleaned-up after (e.g. Closable)? In general, who has the responsibility for the clean-up?
Cheers,
Andree

[Stephan Classen]

Look at guice-persist or onami-persist. They deal wir DB connections which also need to be closed after it has been used.
To do so they both use the concept of UnitOfWork. The application is then responsible for spanning the UnitOfWork around the code which needs the resource.

[Tavian Barnes]

On Friday, 29 April 2016 19:40:44 UTC-4, Andree Surya wrote:

Is it a commonplace to inject a resource that should be cleaned-up after (e.g. Closable)? In general, who has the responsibility for the clean-up?

If the Reader is unscoped, then everybody who injects it gets a different instance, so it's up to the class that injects it to close it.

But in general I'd say binding something like a Reader is questionable.  I'd rather bind something stateless like a Path, which can be used to open a Reader by whoever needs to.

[Luke Sandberg]

+1 to that.

bind something that can give you the resource rather than the resource itself (unless the lifetime is super well defined, like servletoutputstream).  For example,  guava ByteSource/CharSource would work great

--
You received this message because you are subscribed to the Google Groups "google-guice" group.
To unsubscribe from this group and stop receiving emails from it, send an email to google-guice...@googlegroups.com.
To post to this group, send email to google...@googlegroups.com.
Visit this group at https://groups.google.com/group/google-guice.
To view this discussion on the web visit https://groups.google.com/d/msgid/google-guice/f293df1e-0653-4bcf-9841-e1c5dc281838%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Andree Surya]

Thanks for the responses.

"Look at guice-persist or onami-persist. They deal wir DB connections which also need to be closed after it has been used. To do so they both use the concept of UnitOfWork. The application is then responsible for spanning the UnitOfWork around the code which needs the resource."

Thanks for the reference. UnitOfWork is a new concept to me and I'll take some time to read on it.

"But in general I'd say binding something like a Reader is questionable. I'd rather bind something stateless like a Path, which can be used to open a Reader by whoever needs to."

My original intention was to make it easy replace a FileReader with a StringReader during unit test, thus avoiding dependency to the file system. I agree, though, that injecting a stateful object like a Reader could possibly create confusion concerning state management. The Parser depends on the Reader, but because the Reader is injected from outside, somebody else could have messed it up (closing, moving the cursor, etc).

"bind something that can give you the resource rather than the resource itself (unless the lifetime is super well defined, like servletoutputstream). For example, guava ByteSource/CharSource would work great."

Thank you, I think I'll proceed this approach. This way, I can can easily replace file-based input source to an in-memory data during unit test, while encapsulating the responsibility of opening/closing the stream within the Parser object.

Andree

You received this message because you are subscribed to a topic in the Google Groups "google-guice" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/google-guice/rcUWE--TfRQ/unsubscribe.
To unsubscribe from this group and all its topics, send an email to google-guice...@googlegroups.com.

To post to this group, send email to google...@googlegroups.com.
Visit this group at https://groups.google.com/group/google-guice.

To view this discussion on the web visit https://groups.google.com/d/msgid/google-guice/CAO9V1MKVE96AxX6vYftRA92o7ObQn-K%2BO1jDbf4%2B-PeStmqu_g%40mail.gmail.com.

[Stephan Classen]

If this is about testing:
My recomendation is to use constructor injection. And then in the unit test I simply call the constructor. This way I have full controll over what is passed to the subject under test.
As a consequence of this. I do not make use of dependency injection during unit testing. Which then removes the need to bind different objects for production and testing.

Integration testing is different but there it is most of the time sufficient to change bindings to a few external resources such as DB, mail, etc.

[Russ Milliken]

+1

Using constructor injection and not using DI in unit tests is the way to go, for exactly the reasons that Stephan describes.

To view this discussion on the web visit https://groups.google.com/d/msgid/google-guice/B9FE2737-CB62-45D7-A006-71BD8E0EC548%40gmx.ch.

[Andree Surya]

Hi Stephan,

"My recomendation is to use constructor injection. And then in the unit test I simply call the constructor. This way I have full controll over what is passed to the subject under test."

Yes, thank you for the suggestion. By not using dependency injection during unit test, I find the test to be easier to understand (at least in my simple case) because what's being passed in to the SUT is explicitly written.

To view this discussion on the web visit https://groups.google.com/d/msgid/google-guice/CANEiXrpBz84KyBXn91qm_TFdtoBYvfPMsFgEOwa1-q0mPigCGw%40mail.gmail.com.