Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Optimization to log an exception at most once if the same exception i…

…s being

(re-)thrown several times

This used to be an ad-hoc optimization available only in
``ScalaCompilationUnit``.  However, I believe this can be useful in general,
and the optimization belongs to ``EclipseLoggger``.

The former implementation was thread-safe because ``lastCrash`` was always
accessed within the ``PresentationCompilerThread`` (thread-safety was achieved
via thread-confinement). However, it was a weak form of thread-safety, as it
was potentially very easy to break if you didn't understand the thread policy
of the class (which is not documented).  Implementing this optimization in
``EclipseLogger`` forced us to have a stronger form of thread-safety, because
``lastCrash`` can be accessed from any thread.

Documented thread safety guarantees of ``EclipseLogger`` and also described its
synchronization policy for mantainers
  • Loading branch information...
commit 5b3fc47f2ca8503c3e46d50a5f9dcf4023d10d19 1 parent 836bde2
@dotta dotta authored
View
16 org.scala-ide.sdt.core/src/scala/tools/eclipse/javaelements/ScalaCompilationUnit.scala
@@ -41,8 +41,6 @@ trait ScalaCompilationUnit extends Openable with env.ICompilationUnit with Scala
val project = ScalaPlugin.plugin.getScalaProject(getJavaProject.getProject)
val file : AbstractFile
-
- private var lastCrash: Throwable = null
def doWithSourceFile(op : (SourceFile, ScalaPresentationCompiler) => Unit) {
project.withSourceFile(this)(op)(())
@@ -91,7 +89,7 @@ trait ScalaCompilationUnit extends Openable with env.ICompilationUnit with Scala
false
case ex =>
- handleCrash("Compiler crash while building structure for %s".format(file), ex)
+ logger.error("Compiler crash while building structure for %s".format(file), ex)
false
}
}) (false)
@@ -109,14 +107,6 @@ trait ScalaCompilationUnit extends Openable with env.ICompilationUnit with Scala
r.set()
r
}
-
- /** Log an error at most once for this source file. */
- private def handleCrash(msg: String, ex: Throwable) {
- if (lastCrash != ex) {
- lastCrash = ex
- eclipseLog.error(msg, ex)
- }
- }
/** Index this source file, but only if the project has the Scala nature.
*
@@ -130,7 +120,7 @@ trait ScalaCompilationUnit extends Openable with env.ICompilationUnit with Scala
new compiler.IndexBuilderTraverser(indexer).traverse(tree)
}
} catch {
- case ex: Throwable => handleCrash("Compiler crash during indexing of %s".format(getResource()), ex)
+ case ex: Throwable => logger.error("Compiler crash during indexing of %s".format(getResource()), ex)
}
}
}
@@ -235,7 +225,7 @@ trait ScalaCompilationUnit extends Openable with env.ICompilationUnit with Scala
}
} catch {
case ex =>
- handleCrash("Exception thrown while creating override indicators for %s".format(sourceFile), ex)
+ logger.error("Exception thrown while creating override indicators for %s".format(sourceFile), ex)
}
}
}
View
54 org.scala-ide.sdt.core/src/scala/tools/eclipse/logging/EclipseLogger.scala
@@ -5,9 +5,28 @@ import scala.tools.eclipse.ScalaPlugin
import scala.tools.eclipse.util.SWTUtils
import org.eclipse.core.runtime.{ ILog, IStatus }
import scala.util.control.ControlThrowable
+import java.util.concurrent.atomic.AtomicReference
+/** Use the `EclipseLogger` when you want to communicate with the user. Messages logged
+ * through the `EclipseLogger` are persisted in the Error Log.
+ *
+ * This class is meant to be thread-safe, but it isn't because of a possible race condition existing
+ * in the Eclipse Logger. Indeed, the call to `ScalaPlugin.plugin.getLog()` isn't thread-safe, but
+ * there is really nothing we can do about it. The issue is that the `logs` map accessed in
+ * [[org.eclipse.core.internal.runtime.InternalPlatform.getLog(bundle)]] is not synchronized.
+ *
+ * Mantainers should evolve this class by keeping in mind that the `EclipseLogger` will be
+ * accessed by different threads at the same time, so it is important to keep this class lock-free,
+ * and thread-safe (or maybe a better wording would be: as much thread-safe as it can be).
+ * And that is actually the main motivation for using an [[java.util.concurrent.atomic.AtomicReference]]
+ * for `lastCrash`. Also, note that declaring `lastCrash` as volatile (instead of using a
+ * [[java.util.concurrent.atomic.AtomicReference]]) would have made this class less correct (because
+ * volatile fields don't offer atomic operations such as `getAndSet`).
+ */
private[logging] object EclipseLogger extends Logger {
- private final val pluginLogger: ILog = ScalaPlugin.plugin.getLog
+ private val pluginLogger: ILog = ScalaPlugin.plugin.getLog()
+
+ private val lastCrash: AtomicReference[Throwable] = new AtomicReference
def debug(message: => Any) {
info(message)
@@ -48,23 +67,28 @@ private[logging] object EclipseLogger extends Logger {
def fatal(message: => Any, t: Throwable) {
error(message, t)
}
-
+
private def log(severity: Int, message: => Any, t: Throwable = null) {
- // Because of a potential deadlock in the Eclipse internals (look at #1000914), the log action need to be executed in the UI thread.
- def log(status: Status) =
- if (ScalaPlugin.plugin.headlessMode) pluginLogger.log(status)
- else SWTUtils.asyncExec { pluginLogger.log(status) }
-
- log(createStatus(severity, message, t))
- t match {
- // `ControlThrowable` should never (ever!) be caught by user code. If that happens, generate extra noise.
- case ce: ControlThrowable =>
- log(createStatus(IStatus.ERROR, "Incorrectly logged ControlThrowable: " + ce.getClass.getSimpleName + "(" + ce.getMessage + ")", t))
- case _ => () // do nothing
+ if (t == null) logInUiThread(severity, message, t)
+ else {
+ // this is an optimization to log the exception at most once if the same exception is being re-thrown several times.
+ val oldValue = lastCrash.getAndSet(t)
+ if (oldValue != t) {
+ logInUiThread(severity, message, t)
+ t match {
+ // `ControlThrowable` should never (ever!) be caught by user code. If that happens, generate extra noise.
+ case ce: ControlThrowable =>
+ logInUiThread(IStatus.ERROR, "Incorrectly logged ControlThrowable: " + ce.getClass.getSimpleName + "(" + ce.getMessage + ")", t)
+ case _ => () // do nothing
+ }
+ }
}
}
- private def createStatus(severity: Int, message: Any, exception: Throwable): Status = {
- new Status(severity, ScalaPlugin.plugin.getBundle.getSymbolicName, message.toString, exception)
+ // Because of a potential deadlock in the Eclipse internals (look at #1000914), the log action need to be executed in the UI thread.
+ private def logInUiThread(severity: Int, message: Any, exception: Throwable): Unit = {
+ val status = new Status(severity, ScalaPlugin.plugin.getBundle.getSymbolicName, message.toString, exception)
+ if (ScalaPlugin.plugin.headlessMode) pluginLogger.log(status)
+ else SWTUtils.asyncExec { pluginLogger.log(status) }
}
}
Please sign in to comment.
Something went wrong with that request. Please try again.