Skip to content

Commit

Permalink
improvement: add timeout to worksheet evaluation
Browse files Browse the repository at this point in the history
  • Loading branch information
kasiaMarek committed Mar 29, 2024
1 parent 0f9d773 commit 12da73a
Show file tree
Hide file tree
Showing 5 changed files with 113 additions and 20 deletions.
4 changes: 4 additions & 0 deletions docs/troubleshooting/faq.md
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,10 @@ project. If you create your worksheet in the root of your project, you will be
on the default Scala version (2.12.12), and you won't have access to anything
except for the standard lib on your classpath.

## I see spurious errors or worksheet fails to evaluate

Worksheet code is entirely wrapped in a class (`class MdocApp`). The class wrapper is needed as a counter measure to dead locks issues, however the wrapper can cause issues for some valid Scala code snippets. Issues like getting stuck in a infinite loop while evaluating or throwing a `StackOverflowException`. Full description of the issue - [mdoc/#853](https://github.com/scalameta/mdoc/issues/853).

## Ammonite scripts

### How do I use Scala 2.x.x for my script?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1042,6 +1042,11 @@ object Messages {
}
}

def worksheetTimeout: String =
"""|Failed to evaluate worksheet, timeout reached.
|See: https://scalameta.org/metals/docs/troubleshooting/faq#i-see-spurious-errors-or-worksheet-fails-to-evaluate
|""".stripMargin

}

object FileOutOfScalaCliBspScope {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ case class UserConfiguration(
automaticImportBuild: AutoImportBuildKind = AutoImportBuildKind.Off,
scalaCliLauncher: Option[String] = None,
defaultBspToBuildTool: Boolean = false,
worksheetTimeout: Int = 30,
) {

def shouldAutoImportNewProject: Boolean =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ import coursierapi.error.SimpleResolutionError
import mdoc.interfaces.EvaluatedWorksheet
import mdoc.interfaces.Mdoc
import org.eclipse.lsp4j.Hover
import org.eclipse.lsp4j.MessageType
import org.eclipse.lsp4j.Position

/**
Expand Down Expand Up @@ -264,9 +265,18 @@ class WorksheetProvider(
)
}
}
interruptThreadOnCancel(path, result, thread)
val cancellable = toCancellable(thread, result)
interruptThreadOnCancel(path, result, cancellable)
thread.start()
thread.join()
val timeout = userConfig().worksheetTimeout * 1000
thread.join(timeout)
if (!result.isDone()) {
cancellable.cancel()
languageClient.showMessage(
MessageType.Warning,
Messages.worksheetTimeout,
)
}
}
jobs.submit(
result,
Expand All @@ -282,6 +292,32 @@ class WorksheetProvider(
result.asScala.recover(onError)
}

private def toCancellable(
thread: Thread,
result: CompletableFuture[Option[EvaluatedWorksheet]],
): Cancelable = {
// Last resort, if everything else fails we use `Thread.stop()`.
val stopThread = new Runnable {
def run(): Unit = {
if (thread.isAlive()) {
scribe.warn(s"thread stop: ${thread.getName()}")
thread.stop()
}
}
}
new Cancelable {
def cancel() =
if (thread.isAlive()) {
// Canceling a running program. first line of
// defense is `Thread.interrupt()`. Fingers crossed it's enough.
result.complete(None)
threadStopper.schedule(stopThread, 3, TimeUnit.SECONDS)
scribe.warn(s"thread interrupt: ${thread.getName()}")
thread.interrupt()
}
}
}

/**
* Prompts the user to cancel the task after a few seconds.
*
Expand All @@ -292,17 +328,8 @@ class WorksheetProvider(
private def interruptThreadOnCancel(
path: AbsolutePath,
result: CompletableFuture[Option[EvaluatedWorksheet]],
thread: Thread,
cancellable: Cancelable,
): Unit = {
// Last resort, if everything else fails we use `Thread.stop()`.
val stopThread = new Runnable {
def run(): Unit = {
if (thread.isAlive()) {
scribe.warn(s"thread stop: ${thread.getName()}")
thread.stop()
}
}
}
// If the program is running for more than
// `userConfig().worksheetCancelTimeout`, then display a prompt for the user
// to cancel the program.
Expand All @@ -317,14 +344,7 @@ class WorksheetProvider(
)
)
cancel.asScala.foreach { c =>
if (c.cancel && thread.isAlive()) {
// User has requested to cancel a running program. first line of
// defense is `Thread.interrupt()`. Fingers crossed it's enough.
result.complete(None)
threadStopper.schedule(stopThread, 3, TimeUnit.SECONDS)
scribe.warn(s"thread interrupt: ${thread.getName()}")
thread.interrupt()
}
if (c.cancel) cancellable.cancel()
}
result.asScala.onComplete(_ => cancel.cancel(true))
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
package tests.worksheets

import scala.meta.internal.metals.InitializationOptions
import scala.meta.internal.metals.Messages
import scala.meta.internal.metals.UserConfiguration
import scala.meta.internal.metals.{BuildInfo => V}

import tests.BaseLspSuite

class WorksheetInfiniteLoopSuite
extends BaseLspSuite("worksheet-infinite-loop") {

override protected def initializationOptions: Option[InitializationOptions] =
Some(
InitializationOptions.Default.copy(
decorationProvider = Some(true)
)
)

override def userConfig: UserConfiguration =
super.userConfig.copy(
worksheetScreenWidth = 40,
worksheetCancelTimeout = 1,
worksheetTimeout = 4,
)

test("infinite-loop") {
for {
_ <- initialize(
s"""
|/metals.json
|{"a": {"scalaVersion": "${V.scala3}"}}
|/a/src/main/scala/foo/Main.worksheet.sc
|class Animal(age: Int)
|
|class Rabbit(age: Int) extends Animal(age):
| val id = Rabbit.tag
| Rabbit.tag += 1
| def getId = id
|
|object Rabbit:
| val basic = Rabbit(0)
| var tag: Int = 0
|
|val peter = Rabbit(2)
|""".stripMargin
)
_ <- server.didOpen("a/src/main/scala/foo/Main.worksheet.sc")
_ = assertNoDiff(
client.workspaceShowMessages,
Messages.worksheetTimeout,
)
_ <- server.didChange("a/src/main/scala/foo/Main.worksheet.sc")(_ =>
"val a = 1"
)
_ <- server.didSave("a/src/main/scala/foo/Main.worksheet.sc")(identity)
_ = assertNoDiff(
client.syntheticDecorations,
"val a = 1 // : Int = 1",
)
} yield ()
}
}

0 comments on commit 12da73a

Please sign in to comment.