Do file.isEmpty and file.nonEmpty have a resource leak?

[rvanheest]

Introduction
I was looking at the source code of this repository. I think it's very clever how you have hidden resource management for the jStreams that result from functions such as Files.list. As I understand it, a jStream is implicitly converted to a Scala Iterator like this:

better-files/core/src/main/scala/better/files/Implicits.scala

Lines 340 to 342 in 021384b

	private[files] implicit def pathStreamToFiles(files: JStream[Path]): Files =
	files.toAutoClosedIterator.map(File.apply)
	}

The toAutoClosedIterator that is used here is used in that same file, just a couple of lines up:

better-files/core/src/main/scala/better/files/Implicits.scala

Lines 311 to 321 in 021384b

	implicit class JStreamOps[A](stream: JStream[A]) {
	/**
	* Closes this stream when iteration is complete
	* It will NOT close the stream if it is not depleted!
	*
	* @return
	*/
	def toAutoClosedIterator: Iterator[A] =
	stream.autoClosed.flatMap(_.iterator().asScala)
	}

Here is what triggered me in toAutoClosedIterator:

It will NOT close the stream if it is not depleted!

In other words: if you don't iterate through all elements of the Iterator, the resource is not being closed.

Code analysis
For my use case I wanted to check if a directory is not empty. So the easiest thing to do is to use the file.nonEmpty operator.

better-files/core/src/main/scala/better/files/File.scala

Lines 1079 to 1087 in 021384b

	/**
	*
	* @param linkOptions
	* @return for directories, true if it has no children, false otherwise
	* for files, true if it is a 0-byte file, false otherwise
	* else true if it exists, false otherwise
	*/
	def nonEmpty(implicit linkOptions: File.LinkOptions = File.LinkOptions.default): Boolean =
	!isEmpty(linkOptions)

When I trace this call to file.isEmpty, I find this implementation:

better-files/core/src/main/scala/better/files/File.scala

Lines 1065 to 1077 in 021384b

	/**
	* @param linkOptions
	* @return true if file is not present or empty directory or 0-bytes file
	*/
	def isEmpty(implicit linkOptions: File.LinkOptions = File.LinkOptions.default): Boolean = {
	if (isDirectory(linkOptions)) {
	children.isEmpty
	} else if (isRegularFile(linkOptions)) {
	toJava.length() == 0
	} else {
	notExists(linkOptions)
	}
	}

For a directory this calls file.children.isEmpty, where isEmpty for an Iterator is implemented using !hasNext. Note that file.children implicitly uses the toAutoClosedIterator with its 'NOT closed if not depleted' statement.

Question
This leads to the following question: do operators like file.isEmpty and file.nonEmpty close the underlying jStream that is produced in file.children? After all, they do not deplete the stream with a single hasNext call. Therefore: do operators like file.isEmpty and file.nonEmpty cause resource leaks?

[rvanheest]

I further looked into this by means of experiments:

1. in IntelliJ I put a breakpoint on the _.close lambda in

   better-files/core/src/main/scala/better/files/ManagedResource.scala

   Line 32 in 021384b

   Disposable(_.close())

2. using the code below, I ran the debugger.

val file = // some path to a directory
println(file.exists) // should print 'true'

// case 1
file.children.toList

// case 2
file.children.take(1).toList // note: the directory has more than 1 child

// case 3
file.nonEmpty

For case 1 I found that the breakpoint on _.close was hit. However, neither in case 2 nor in case 3 the breakpoint was hit. This is (most probably) due to the underlying Iterator not being fully depleted.

I also put similar breakpoints at other lines that I expected the 'dispose process' to hit and none of them where ever hit in cases 2 and 3, while being hit in case 1.

[rvanheest]

When can we expect this bug fix to be released?

[pathikrit]

@rvanheest : You can either depend on the snapshot versions for now or you can simply depend on a git hash:

.dependsOn(ProjectRef(uri("git://github.com/pathikrit/better-files.git#git-hash"))