Skip to content

Commit

Permalink
Make PositionImpl thread-safe
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
eed3si9n committed Dec 8, 2017
1 parent de29ead commit 5dd777a
Show file tree
Hide file tree
Showing 2 changed files with 104 additions and 66 deletions.
Original file line number Diff line number Diff line change
@@ -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")
Original file line number Diff line number Diff line change
Expand Up @@ -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")

Expand Down Expand Up @@ -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)
}
}
}

0 comments on commit 5dd777a

Please sign in to comment.