2

After opening a file, someone has to close it.

Kotlin has the convenient use { ... } method for the case where you're doing all the work with the file in-place.

But sometimes, you have to do some other things with the file and return another object which gains ownership.

Currently, I am doing something like this:

    fun open(path: Path, fileMode: FileMode): StoreFile {
        var success = false
        val channel = FileChannelIO.open(path, fileMode)
        try {
            if (channel.size == 0 && fileMode == FileMode.READ_WRITE) {
                writeInitialContent(channel)
            }

            val result = StoreFile(channel)
            success = true
            return result
        } finally {
            if (!success) {
                channel.close()
            }
        }
    }

writeInitialContent could fail, in which case I would have to close the file. If it doesn't fail, then StoreFile takes ownership and becomes the thing which the caller has to close.

This is a common kind of nastiness in Java, and the way I've had to write it in Kotlin isn't really any cleaner than Java, and with the way it's currently written, there's still the potential for double-closing if StoreFile itself also happens to close the file on failure.

You hit the same basic problem even trying to buffer a stream, which is also fairly common. In this case, I don't know whether buffered() will throw, and if it does, nobody closes the file. In this case, buffered() itself could deal with the ugliness, but I checked its code and it does not.

    fun readSomething(path: Path): Something {
        path.inputStream().buffered().use { stream ->
            // ...
        }
    }

Is there a better way to structure this?

Hakanai
  • 12,010
  • 10
  • 62
  • 132
  • 3
    I don't know of a way to make this cleaner, but I think a general rule everyone should follow is that only the function that opens a closable should be the one that closes it. Any function that takes a closable as an argument has no business closing it. – Tenfour04 Nov 30 '22 at 00:59
  • I don't have a better suggestion other than defining a policy like the Resource creator/opener should close it. Of course, many Resources behave unexpectedly if you access them when closed (exception being StringWriter), so your downstream users should not close. Another approach is present an interface to downstream users which has no option to close. Perhaps a `Sequence`? – AndrewL Nov 30 '22 at 01:05
  • 1
    Create a method with a callback - a method like use. – Louis Wasserman Nov 30 '22 at 05:16
  • @LouisWasserman as an alternative to returning the `StoreFile`? I am kind of fond of that sort of API myself as it guarantees that nobody can forget to close it, but some callers will complain that they want to create the object from one place and then close it somewhere else. It might work well for the current situation though. – Hakanai Dec 02 '22 at 09:04
  • @Tenfour04 a corollary to that rule is that no method should ever return an `AutoCloseable`, I guess... so all methods have to be pivoted to the callback style. It doesn't seem to be a very popular way to do things though, whether I look at the standard library, or at much newer I/O libraries like Okio. Of course, if I take this rule to the extreme - it results in "AutoCloseable should not exist" - because if you never need to return it, and since everything is closed by the method that created it, you never need it. – Hakanai Dec 02 '22 at 09:08

0 Answers0