Navigation Menu

Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid file leaks when digesting Gradle builds #889

Merged
merged 4 commits into from Sep 2, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -34,8 +34,13 @@ final class BuildTools(
dir.isDirectory && {
val ls = Files.list(dir.toNIO)
val hasJsonFile =
ls.iterator().asScala.exists(_.getFileName.toString.endsWith(".json"))
ls.close()
try {
ls.iterator()
.asScala
.exists(_.getFileName.toString.endsWith(".json"))
} finally {
ls.close()
}
hasJsonFile
}
}
Expand Down
7 changes: 5 additions & 2 deletions metals/src/main/scala/scala/meta/internal/builds/Digest.scala
Expand Up @@ -13,6 +13,7 @@ import scala.meta.tokens.Token
import scala.util.control.NonFatal
import scala.meta.internal.jdk.CollectionConverters._
import scala.xml.Node
import scala.meta.internal.mtags.ListFiles

case class Digest(
md5: String,
Expand Down Expand Up @@ -62,9 +63,11 @@ object Digest {
): Boolean = {
if (!path.isDirectory) true
else {
Files.list(path.toNIO).iterator().asScala.forall { file =>
digestFile(AbsolutePath(file), digest)
var success = true
ListFiles.foreach(path) { file =>
success &= digestFile(file, digest)
}
success
}
}

Expand Down
@@ -1,10 +1,12 @@
package scala.meta.internal.builds

import scala.meta.io.AbsolutePath
import scala.meta.internal.mtags.WalkFiles
import scala.meta.internal.jdk.CollectionConverters._
import java.security.MessageDigest
import java.nio.file.Files
import java.util.stream.Collectors
import java.nio.file.Path
import scala.meta.internal.jdk.CollectionConverters._

object GradleDigest extends Digestable {
override protected def digestWorkspace(
Expand All @@ -24,9 +26,10 @@ object GradleDigest extends Digestable {
}

def digestBuildSrc(path: AbsolutePath, digest: MessageDigest): Boolean = {
Files.walk(path.toNIO).iterator().asScala.forall { file =>
Digest.digestFile(AbsolutePath(file), digest)
WalkFiles.foreach(path) { file =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to have the forall method in ListFiles, returning true doesn't look good.

Digest.digestFile(file, digest)
}
true
}

def digestSubProjects(
Expand Down
@@ -1,20 +1,21 @@
package scala.meta.internal.builds
import java.nio.file.Files

import java.security.MessageDigest
import scala.meta.io.AbsolutePath
import scala.meta.internal.mtags.WalkFiles
import scala.meta.internal.mtags.MtagsEnrichments._

object MavenDigest extends Digestable {
override protected def digestWorkspace(
workspace: AbsolutePath,
digest: MessageDigest
): Boolean = {
Digest.digestFile(workspace.resolve("pom.xml"), digest)
Files.walk(workspace.toNIO).allMatch { file =>
if (file.getFileName.toString == "pom.xml") {
Digest.digestFile(AbsolutePath(file), digest)
} else {
true
WalkFiles.foreach(workspace) { file =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here it would be nicer to just have forall.

if (file.filename == "pom.xml") {
Digest.digestFile(file, digest)
}
}
true
}
}
16 changes: 7 additions & 9 deletions metals/src/main/scala/scala/meta/internal/builds/SbtDigest.scala
@@ -1,15 +1,13 @@
package scala.meta.internal.builds

import java.nio.file.{Files, Path}
import java.security.MessageDigest

import scala.meta.internal.jdk.CollectionConverters._
import scala.meta.internal.builds.Digest.digestScala
import scala.meta.internal.io.PathIO
import scala.meta.internal.metals.MetalsEnrichments._
import scala.meta.io.AbsolutePath
import scala.meta.internal.mtags.ListFiles

object SbtDigest extends Digestable {
val sbtExtension = "sbt"

override protected def digestWorkspace(
workspace: AbsolutePath,
Expand All @@ -29,14 +27,14 @@ object SbtDigest extends Digestable {
if (!path.isDirectory) {
true
} else {
Files.list(path.toNIO).iterator().asScala.forall(digestSbtFile(digest))
var success = true
ListFiles.foreach(path)(file => success &= digestSbtFile(digest)(file))
success
}
}

private def digestSbtFile(digest: MessageDigest)(filePath: Path) = {
val path = AbsolutePath(filePath)
def ext = PathIO.extension(path.toNIO)
if (path.isFile && ext == sbtExtension) {
private def digestSbtFile(digest: MessageDigest)(path: AbsolutePath) = {
if (path.isFile && path.extension == "sbt") {
digestScala(path, digest)
} else {
true
Expand Down
Expand Up @@ -5,17 +5,16 @@ import com.google.gson.Gson
import io.github.soc.directories.ProjectDirectories
import java.nio.charset.Charset
import java.nio.charset.StandardCharsets
import java.nio.file.Files
import java.security.MessageDigest
import scala.concurrent.ExecutionContextExecutorService
import scala.concurrent.Future
import scala.meta.internal.io.FileIO
import scala.meta.internal.io.PathIO
import scala.meta.internal.metals.Messages.BspSwitch
import scala.meta.internal.metals.MetalsEnrichments._
import scala.meta.internal.mtags.MD5
import scala.meta.io.AbsolutePath
import scala.util.Try
import scala.meta.internal.mtags.ListFiles

/**
* Implements BSP server discovery, named "BSP Connection Protocol" in the spec.
Expand Down Expand Up @@ -126,9 +125,9 @@ final class BspServers(
val buf = List.newBuilder[AbsolutePath]
def visit(dir: AbsolutePath): Unit =
if (dir.isDirectory) {
Files.list(dir.toNIO).iterator().asScala.foreach { p =>
if (PathIO.extension(p) == "json") {
buf += AbsolutePath(p)
ListFiles.foreach(dir) { p =>
if (p.extension == "json") {
buf += p
}
}
}
Expand Down
Expand Up @@ -216,8 +216,6 @@ object MetalsEnrichments
}
implicit class XtensionAbsolutePathBuffers(path: AbsolutePath) {

def filename: String = path.toNIO.getFileName.toString

def sourcerootOption: String = s""""-P:semanticdb:sourceroot:$path""""

/**
Expand Down
Expand Up @@ -1304,7 +1304,7 @@ class MetalsLanguageServer(
private def indexWorkspaceSources(): Unit = {
for {
(sourceItem, targets) <- buildTargets.sourceItemsToBuildTargets
source <- ListFiles(sourceItem)
source <- WalkFiles(sourceItem)
if source.isScalaOrJava
} {
targets.asScala.foreach { target =>
Expand Down
Expand Up @@ -178,7 +178,7 @@ class ClasspathSymbols(isStatisticsEnabled: Boolean = false) {
if (Files.isDirectory(path)) {
val ls = Files.list(path)
try ls.collect(Collectors.toList()).asScala
finally ls.close
finally ls.close()
} else {
Nil
}
Expand Down
Expand Up @@ -20,6 +20,7 @@ import scala.util.control.NonFatal
import scala.util.Properties
import java.nio.file.FileSystems
import java.net.URI
import scala.meta.internal.mtags.ListFiles

/**
* An index to lookup classfiles contained in a given classpath.
Expand Down Expand Up @@ -136,8 +137,9 @@ class PackageIndex() {
for {
pkg <- Files.newDirectoryStream(dir).iterator().asScala
symbol = pkg.toString.stripPrefix("/packages/").replace('.', '/') + "/"
moduleLink <- Files.list(pkg).iterator().asScala
absoluteModuleLink <- ListFiles(AbsolutePath(pkg))
} {
val moduleLink = absoluteModuleLink.toNIO
val module =
if (!Files.isSymbolicLink(moduleLink)) moduleLink
else Files.readSymbolicLink(moduleLink)
Expand Down
Expand Up @@ -31,10 +31,13 @@ object RecursivelyDelete {
exc: IOException
): FileVisitResult = {
val stream = Files.list(dir)
if (!stream.iterator().hasNext) {
Files.delete(dir)
try {
if (!stream.iterator().hasNext) {
Files.delete(dir)
}
} finally {
stream.close()
}
stream.close()
super.postVisitDirectory(dir, exc)
}
}
Expand Down
51 changes: 11 additions & 40 deletions mtags/src/main/scala/scala/meta/internal/mtags/ListFiles.scala
@@ -1,50 +1,21 @@
package scala.meta.internal.mtags

import java.nio.file.FileVisitResult
import java.nio.file.Files
import java.nio.file.NoSuchFileException
import java.nio.file.Path
import java.nio.file.SimpleFileVisitor
import java.nio.file.attribute.BasicFileAttributes
import scala.meta.io.AbsolutePath
import scala.collection.mutable.ArrayBuffer
import scala.collection.mutable

object ListFiles {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name can be misleading, as it lists both files and directories

def foreach(root: AbsolutePath)(fn: AbsolutePath => Unit): Unit = {
ListFiles(root).foreach(fn)
def apply(root: AbsolutePath): mutable.ArrayBuffer[AbsolutePath] = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be in an extension method for AbsolutePath, like list or children

val buf = mutable.ArrayBuffer.empty[AbsolutePath]
foreach(root)(file => buf += file)
buf
}

def apply(root: AbsolutePath): ArrayBuffer[AbsolutePath] = {

def runForeach[U](fn: AbsolutePath => U): Unit = {
if (root.isFile) fn(root)
else if (root.isDirectory) {
try {
Files.walkFileTree(
root.toNIO,
new SimpleFileVisitor[Path] {
override def visitFile(
file: Path,
attrs: BasicFileAttributes
): FileVisitResult = {
fn(AbsolutePath(file))
super.visitFile(file, attrs)
}
}
)
} catch {
case _: NoSuchFileException =>
() // error is reported by the JDK
}
}
}

if (root.isFile) {
ArrayBuffer(root)
} else {
val buf = ArrayBuffer.empty[AbsolutePath]
runForeach(file => { buf += file })
buf
def foreach(root: AbsolutePath)(fn: AbsolutePath => Unit): Unit = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be useful to have the couple of methods to use like map or forall or exist. Shouldn't be difficult to do.

val ls = Files.list(root.toNIO)
try {
ls.forEach(file => fn(AbsolutePath(file)))
} finally {
ls.close()
}
}
}
Expand Up @@ -74,6 +74,9 @@ trait MtagsEnrichments {
}
}
implicit class XtensionAbsolutePathMetals(file: AbsolutePath) {

def filename: String = file.toNIO.filename

def toIdeallyRelativeURI(sourceItemOpt: Option[AbsolutePath]): String =
sourceItemOpt match {
case Some(sourceItem) =>
Expand Down
Expand Up @@ -53,7 +53,7 @@ final case class OnDemandSymbolIndex(
override def addSourceJar(jar: AbsolutePath): Unit = tryRun {
if (sourceJars.addEntry(jar)) {
FileIO.withJarFileSystem(jar, create = false) { root =>
ListFiles.foreach(root) { source =>
WalkFiles.foreach(root) { source =>
if (source.toLanguage.isScala) {
try {
addSourceFile(source, None)
Expand Down
41 changes: 41 additions & 0 deletions mtags/src/main/scala/scala/meta/internal/mtags/WalkFiles.scala
@@ -0,0 +1,41 @@
package scala.meta.internal.mtags

import java.nio.file.FileVisitResult
import java.nio.file.Files
import java.nio.file.NoSuchFileException
import java.nio.file.Path
import java.nio.file.SimpleFileVisitor
import java.nio.file.attribute.BasicFileAttributes
import scala.meta.io.AbsolutePath
import scala.collection.mutable.ArrayBuffer

object WalkFiles {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add to extension method? It's really hidden in here, if someone doesn't know about this then they will use something else.

def foreach(root: AbsolutePath)(fn: AbsolutePath => Unit): Unit = {
if (root.isFile) fn(root)
else if (root.isDirectory) {
try {
Files.walkFileTree(
root.toNIO,
new SimpleFileVisitor[Path] {
override def visitFile(
file: Path,
attrs: BasicFileAttributes
): FileVisitResult = {
fn(AbsolutePath(file))
FileVisitResult.CONTINUE
}
}
)
} catch {
case _: NoSuchFileException =>
() // error is reported by the JDK
}
}
}

def apply(root: AbsolutePath): ArrayBuffer[AbsolutePath] = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be an extension method for AbsolutePath, like walk or listRecursively or descendants or event childrenRecursively

val buf = ArrayBuffer.empty[AbsolutePath]
foreach(root)(file => buf += file)
buf
}
}
2 changes: 1 addition & 1 deletion project/plugins.sbt
Expand Up @@ -2,7 +2,7 @@ addSbtPlugin("pl.project13.scala" % "sbt-jmh" % "0.3.4")
addSbtPlugin("ch.epfl.scala" % "sbt-scalafix" % "0.9.2")
addSbtPlugin("com.geirsson" % "sbt-ci-release" % "1.2.2")
addSbtPlugin("org.scalameta" % "sbt-mdoc" % "1.2.7")
addSbtPlugin("com.eed3si9n" % "sbt-buildinfo" % "0.7.0")
addSbtPlugin("com.eed3si9n" % "sbt-buildinfo" % "0.9.0")

addSbtCoursier

Expand Down
14 changes: 7 additions & 7 deletions tests/unit/src/main/scala/tests/QuickBuild.scala
Expand Up @@ -14,14 +14,14 @@ import java.nio.file.Path
import java.nio.file.Paths
import java.security.MessageDigest
import scala.meta.internal.io.FileIO
import scala.meta.internal.io.PathIO
import scala.meta.internal.metals.MetalsEnrichments._
import scala.meta.internal.metals.Time
import scala.meta.internal.metals.Timer
import scala.meta.internal.metals.{BuildInfo => V}
import scala.meta.internal.mtags.MD5
import scala.meta.io.AbsolutePath
import scala.util.matching.Regex
import scala.meta.internal.mtags.ListFiles

/**
* A basic build tool for faster testing.
Expand Down Expand Up @@ -269,9 +269,9 @@ object QuickBuild {
update(workspace.resolve("metals.json"))
val bloopDirectory = workspace.resolve(".bloop").toNIO
Files.createDirectories(bloopDirectory)
Files.list(bloopDirectory).forEach { path =>
if (PathIO.extension(path) == "json") {
update(AbsolutePath(path))
ListFiles.foreach(AbsolutePath(bloopDirectory)) { path =>
if (path.extension == "json") {
update(path)
}
}
MD5.bytesToHex(digest.digest())
Expand All @@ -297,9 +297,9 @@ object QuickBuild {
}
val bloopDirectory = workspace.resolve(".bloop").toNIO
Files.createDirectories(bloopDirectory)
Files.list(bloopDirectory).forEach { path =>
if (PathIO.extension(path) == "json") {
Files.delete(path)
ListFiles.foreach(AbsolutePath(bloopDirectory)) { path =>
if (path.extension == "json") {
path.delete()
}
}
val bloopProjects = projects.map(_.toBloop(workspace))
Expand Down