From 5dd777ab8c53b7d23942af9cc3bcc3de0929eee1 Mon Sep 17 00:00:00 2001 From: Eugene Yokota Date: Tue, 5 Dec 2017 22:14:04 -0500 Subject: [PATCH] Make PositionImpl thread-safe Fixes #464 sbt/sbt#3623 Currently compiling a lot of Java code in CI environment causes NullPointerException, which is suspected of race condition around `javax.tools.Diagnostics[S]`, which is held by `PositionImpl` and then later accessed by async logging. This change makes the `PositionImpl` strict and immutable, and extracts it from a Java Diagnostics object. No other observable changes are introduced besides, hopefully the lack of NPE. Credit on the detective work goes to Lightbend Akka team ktoso, 2m, and jrudolph. --- .../mima-filters/1.0.0.backwards.excludes | 3 + .../inc/javac/DiagnosticsReporter.scala | 167 +++++++++++------- 2 files changed, 104 insertions(+), 66 deletions(-) create mode 100644 internal/zinc-compile-core/src/main/mima-filters/1.0.0.backwards.excludes diff --git a/internal/zinc-compile-core/src/main/mima-filters/1.0.0.backwards.excludes b/internal/zinc-compile-core/src/main/mima-filters/1.0.0.backwards.excludes new file mode 100644 index 0000000000..bda5e13bac --- /dev/null +++ b/internal/zinc-compile-core/src/main/mima-filters/1.0.0.backwards.excludes @@ -0,0 +1,3 @@ +# PositionImpl is a private class only invoked in the same source. +ProblemFilters.exclude[FinalClassProblem]("sbt.internal.inc.javac.DiagnosticsReporter$PositionImpl") +ProblemFilters.exclude[DirectMissingMethodProblem]("sbt.internal.inc.javac.DiagnosticsReporter#PositionImpl.this") diff --git a/internal/zinc-compile-core/src/main/scala/sbt/internal/inc/javac/DiagnosticsReporter.scala b/internal/zinc-compile-core/src/main/scala/sbt/internal/inc/javac/DiagnosticsReporter.scala index c96e6a6a81..c4af81a243 100644 --- a/internal/zinc-compile-core/src/main/scala/sbt/internal/inc/javac/DiagnosticsReporter.scala +++ b/internal/zinc-compile-core/src/main/scala/sbt/internal/inc/javac/DiagnosticsReporter.scala @@ -23,6 +23,8 @@ import javax.tools.Diagnostic.NOPOS * @param reporter */ final class DiagnosticsReporter(reporter: Reporter) extends DiagnosticListener[JavaFileObject] { + import DiagnosticsReporter._ + val END_OF_LINE_MATCHER = "(\r\n)|[\r]|[\n]" val EOL = System.getProperty("line.separator") @@ -64,85 +66,118 @@ final class DiagnosticsReporter(reporter: Reporter) extends DiagnosticListener[J import sbt.util.InterfaceUtil.problem val msg = fixedDiagnosticMessage(d) - val pos: xsbti.Position = new PositionImpl(d) + val pos: xsbti.Position = PositionImpl(d) if (severity == Severity.Error) errorEncountered = true reporter.log(problem("", pos, msg, severity)) } +} - private class PositionImpl(d: Diagnostic[_ <: JavaFileObject]) extends xsbti.Position { - // https://docs.oracle.com/javase/7/docs/api/javax/tools/Diagnostic.html - // Negative values (except NOPOS) and 0 are not valid line or column numbers, - // except that you can cause this number to occur by putting "abc {}" in A.java. - // This will cause Invalid position: 0 masking the actual error message - // a/A.java:1: class, interface, or enum expected - private[this] def checkNoPos(n: Long): Option[Long] = - n match { - case NOPOS => None - case x if x <= 0 => None - case x => Option(x) - } - - override val line: Optional[Integer] = o2jo(checkNoPos(d.getLineNumber) map { x => - new Integer(x.toInt) - }) - def startPosition: Option[Long] = checkNoPos(d.getStartPosition) - def endPosition: Option[Long] = checkNoPos(d.getEndPosition) - override val offset: Optional[Integer] = o2jo(checkNoPos(d.getPosition) map { x => - new Integer(x.toInt) - }) - override def lineContent: String = { - def getDiagnosticLine: Option[String] = - try { - // See com.sun.tools.javac.api.ClientCodeWrapper.DiagnosticSourceUnwrapper - val diagnostic = d.getClass.getField("d").get(d) - // See com.sun.tools.javac.util.JCDiagnostic#getDiagnosticSource - val getDiagnosticSourceMethod = - diagnostic.getClass.getDeclaredMethod("getDiagnosticSource") - val getPositionMethod = diagnostic.getClass.getDeclaredMethod("getPosition") - (Option(getDiagnosticSourceMethod.invoke(diagnostic)), - Option(getPositionMethod.invoke(diagnostic))) match { - case (Some(diagnosticSource), Some(position: java.lang.Long)) => - // See com.sun.tools.javac.util.DiagnosticSource - val getLineMethod = diagnosticSource.getClass.getMethod("getLine", Integer.TYPE) - Option(getLineMethod.invoke(diagnosticSource, new Integer(position.intValue()))) - .map(_.toString) - case _ => None - } - } catch { - // TODO - catch ReflectiveOperationException once sbt is migrated to JDK7 - case ignored: Throwable => None - } - - def getExpression: String = - Option(d.getSource) match { - case Some(source: JavaFileObject) => - (Option(source.getCharContent(true)), startPosition, endPosition) match { - case (Some(cc), Some(start), Some(end)) => - cc.subSequence(start.toInt, end.toInt).toString - case _ => "" - } - case _ => "" - } +object DiagnosticsReporter { - getDiagnosticLine.getOrElse(getExpression) - } - private val sourceUri: Option[String] = fixSource(d.getSource) + /** + * Strict and immutable implementation of Position. + */ + private[sbt] final class PositionImpl private ( + sourceUri: Option[String], + override val line: Optional[Integer], + override val lineContent: String, + override val offset: Optional[Integer], + val startPosition: Option[Long], + val endPosition: Option[Long] + ) extends xsbti.Position { override val sourcePath: Optional[String] = o2jo(sourceUri) override val sourceFile: Optional[File] = o2jo(sourceUri.map(new File(_))) override val pointer: Optional[Integer] = o2jo(Option.empty[Integer]) override val pointerSpace: Optional[String] = o2jo(Option.empty[String]) + override def toString: String = if (sourceUri.isDefined) s"${sourceUri.get}:${if (line.isPresent) line.get else -1}" else "" - private def fixSource[T <: JavaFileObject](source: T): Option[String] = { - try Option(source).map(_.toUri.normalize).map(new File(_)).map(_.getAbsolutePath) - catch { - case t: IllegalArgumentException => - // Oracle JDK6 has a super dumb notion of what a URI is. In fact, it's not even a legimitate URL, but a dump - // of the filename in a "I hope this works to toString it" kind of way. This appears to work in practice - // but we may need to re-evaluate. - Option(source).map(_.toUri.toString) + } + + private[sbt] object PositionImpl { + + /** + * Extracts PositionImpl from a Java Diagnostic. + * The previous implementation of PositionImpl held on to the Diagostic object as a wrapper, + * and calculated the lineContent on the fly. + * This caused race condition on the Diagnostic object, resulting to NullPointerException. + * See https://github.com/sbt/sbt/issues/3623 + */ + def apply(d: Diagnostic[_ <: JavaFileObject]): PositionImpl = { + // https://docs.oracle.com/javase/7/docs/api/javax/tools/Diagnostic.html + // Negative values (except NOPOS) and 0 are not valid line or column numbers, + // except that you can cause this number to occur by putting "abc {}" in A.java. + // This will cause Invalid position: 0 masking the actual error message + // a/A.java:1: class, interface, or enum expected + def checkNoPos(n: Long): Option[Long] = + n match { + case NOPOS => None + case x if x <= 0 => None + case x => Option(x) + } + + val line: Optional[Integer] = o2jo(checkNoPos(d.getLineNumber) map { x => + new Integer(x.toInt) + }) + val startPosition: Option[Long] = checkNoPos(d.getStartPosition) + val endPosition: Option[Long] = checkNoPos(d.getEndPosition) + val offset: Optional[Integer] = o2jo(checkNoPos(d.getPosition) map { x => + new Integer(x.toInt) + }) + def lineContent: String = { + def getDiagnosticLine: Option[String] = + try { + // See com.sun.tools.javac.api.ClientCodeWrapper.DiagnosticSourceUnwrapper + val diagnostic = d.getClass.getField("d").get(d) + // See com.sun.tools.javac.util.JCDiagnostic#getDiagnosticSource + val getDiagnosticSourceMethod = + diagnostic.getClass.getDeclaredMethod("getDiagnosticSource") + val getPositionMethod = diagnostic.getClass.getDeclaredMethod("getPosition") + (Option(getDiagnosticSourceMethod.invoke(diagnostic)), + Option(getPositionMethod.invoke(diagnostic))) match { + case (Some(diagnosticSource), Some(position: java.lang.Long)) => + // See com.sun.tools.javac.util.DiagnosticSource + val getLineMethod = diagnosticSource.getClass.getMethod("getLine", Integer.TYPE) + Option(getLineMethod.invoke(diagnosticSource, new Integer(position.intValue()))) + .map(_.toString) + case _ => None + } + } catch { + // TODO - catch ReflectiveOperationException once sbt is migrated to JDK7 + case _: Throwable => None + } + + def getExpression: String = + Option(d.getSource) match { + case Some(source: JavaFileObject) => + (Option(source.getCharContent(true)), startPosition, endPosition) match { + case (Some(cc), Some(start), Some(end)) => + cc.subSequence(start.toInt, end.toInt).toString + case _ => "" + } + case _ => "" + } + + getDiagnosticLine.getOrElse(getExpression) + } + + def fixSource[T <: JavaFileObject](source: T): Option[String] = { + try Option(source).map(_.toUri.normalize).map(new File(_)).map(_.getAbsolutePath) + catch { + case t: IllegalArgumentException => + // Oracle JDK6 has a super dumb notion of what a URI is. In fact, it's not even a legimitate URL, but a dump + // of the filename in a "I hope this works to toString it" kind of way. This appears to work in practice + // but we may need to re-evaluate. + Option(source).map(_.toUri.toString) + } } + new PositionImpl(fixSource(d.getSource), + line, + lineContent, + offset, + startPosition, + endPosition) } } }