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

Conversation

@olafurpg
Copy link
Member

commented Sep 1, 2019

Should fix a bug reported on Gitter https://gitter.im/scalameta/metals?at=5d68316b2f10da7cb357a62e

Would be good to include this fix in the upcoming release

cc/ @several27

@olafurpg olafurpg requested a review from tgodzik Sep 1, 2019
@@ -24,7 +24,7 @@ object GradleDigest extends Digestable {
}

def digestBuildSrc(path: AbsolutePath, digest: MessageDigest): Boolean = {
Files.walk(path.toNIO).iterator().asScala.forall { file =>
Files.walk(path.toNIO).allMatch { file =>

This comment has been minimized.

Copy link
@tgodzik

tgodzik Sep 1, 2019

Collaborator

Both .walk and list mention:

     * <p> The returned stream encapsulates one or more {@link DirectoryStream}s.
     * If timely disposal of file system resources is required, the
     * {@code try}-with-resources construct should be used to ensure that the
     * stream's {@link Stream#close close} method is invoked after the stream
     * operations are completed.  Operating on a closed stream will result in an
     * {@link java.lang.IllegalStateException}.

I am wondering if the issue is that we don't call close() on those methods. digestBuildSrc is not used very often, since that is used to define plugins in-build, so I suspect this is not the issue.

This comment has been minimized.

Copy link
@tgodzik

tgodzik Sep 1, 2019

Collaborator

Although, I might be completely wrong here.

@tgodzik
tgodzik approved these changes Sep 1, 2019
olafurpg added 4 commits Sep 1, 2019
It seems like `Files.walk(...).allMatch` also leaks so we replace it
with `Files.walkFileTree` instead.
Always ensure we close the returned stream. Using something like
lihaoyi/os-lib would be nice, although it would be great if it used
`nio.file.Path` instead of `os.Path`.
CI has been failing repeatedly with "failed to copy ...
BuildInfo.scala".
@olafurpg olafurpg force-pushed the olafurpg:leaky-files branch from dd43dcf to 1d94d47 Sep 1, 2019
@olafurpg olafurpg requested a review from tgodzik Sep 1, 2019
@olafurpg

This comment has been minimized.

Copy link
Member Author

commented Sep 1, 2019

Flaky tests.DefinitionSlowSuite failure in CI

@tgodzik
tgodzik approved these changes Sep 2, 2019
Copy link
Collaborator

left a comment

I think there are some things we could do here, I don't really like returning true in some places, we could use a couple more methods than just foreach. Also we should keep most things in the extension methods, so that they are easy to find.

We can merge this since we need it before the release, but I think this should really be reworked later on.

val buf = ArrayBuffer.empty[AbsolutePath]
runForeach(file => { buf += file })
buf
def foreach(root: AbsolutePath)(fn: AbsolutePath => Unit): Unit = {

This comment has been minimized.

Copy link
@tgodzik

tgodzik Sep 2, 2019

Collaborator

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

import scala.meta.io.AbsolutePath
import scala.collection.mutable.ArrayBuffer

object WalkFiles {

This comment has been minimized.

Copy link
@tgodzik

tgodzik Sep 2, 2019

Collaborator

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

@@ -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 =>

This comment has been minimized.

Copy link
@tgodzik

tgodzik Sep 2, 2019

Collaborator

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

Digest.digestFile(AbsolutePath(file), digest)
} else {
true
WalkFiles.foreach(workspace) { file =>

This comment has been minimized.

Copy link
@tgodzik

tgodzik Sep 2, 2019

Collaborator

Also here it would be nicer to just have forall.

Copy link
Collaborator

left a comment

As a quick fix, this change is OK. In the long run, the complexity it adds will have to be dealt with. We should probably open an issue when this gets merge to not forget.

The issue causing the accidental complexity is the fact that the foreach is not powerful enough to meet our needs, hence the need to reimplement the map and exists methods at call site.

Instead, I would propose something more along the lines of:

implicit class XtensionJavaStream[A](stream: Stream[A]){
  def consume[B](f: Iterator[A] => B) = {
    try f(stream.iterator.asScala)
    finally stream.close()
  }

implicit class AbsolutePathExtension(p){
  def walk: Seq[AbsolutePath] = Files.walk(p).consume{ files =>
    f.map(AbsolutePath(_))
  }
}

Then, the caller wouldn't have to deal with closing of the resources and the code at the call site would get a lot simpler:
This would make the code a bit simpler:

workspace.walk
  .filter(_.filename == "pom.xml")
  .forall(Digest.digestFile(_, digest))

instead of

WalkFiles.foreach(workspace) { file =>
  if (file.filename == "pom.xml") {
    Digest.digestFile(file, digest)
  }
}
true

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

This comment has been minimized.

Copy link
@marek1840

marek1840 Sep 2, 2019

Collaborator

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

}
}

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

This comment has been minimized.

Copy link
@marek1840

marek1840 Sep 2, 2019

Collaborator

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

import scala.meta.io.AbsolutePath
import scala.collection.mutable.ArrayBuffer
import scala.collection.mutable

object ListFiles {

This comment has been minimized.

Copy link
@marek1840

marek1840 Sep 2, 2019

Collaborator

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

@tgodzik

This comment has been minimized.

Copy link
Collaborator

commented Sep 2, 2019

Flaky tests.DefinitionSlowSuite failure in CI

I think it didn't get fixed unfortunately in your PR :/

@tgodzik

This comment has been minimized.

Copy link
Collaborator

commented Sep 2, 2019

Let's merge this as is, but I will add a suggestion to the issue about code cleanup.

@tgodzik tgodzik merged commit 1d058b4 into scalameta:master Sep 2, 2019
1 of 2 checks passed
1 of 2 checks passed
scalameta.metals Build #20190901.5 failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tgodzik tgodzik referenced this pull request Sep 2, 2019
@marek1840

This comment has been minimized.

Copy link
Collaborator

commented Sep 2, 2019

As it turns out, this doesn't fix the core issue, which is that the resources are not being closed inside the GradleDigest::digestSubProjects method. @tgodzik is working on a follow up.

@olafurpg olafurpg deleted the olafurpg:leaky-files branch Sep 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.