Compiler cancellation with new compiler job queue #23191
Conversation
| case Some(path) => | ||
| logger.log(Level.SEVERE, s"$shortMessage. Full details of the error can be found in the error report $path") | ||
| case _ => | ||
| logger.log(Level.SEVERE, shortMessage + "\n" + stacktrace.indent(2)) |
There was a problem hiding this comment.
I think the indent method is not avialable for JDK 8
| object CompilationInputs: | ||
| def empty: CompilationInputs = CompilationInputs(new URI(""), "", EmptyCancelToken, false) | ||
| def fromParams(params: VirtualFileParams | OffsetParams, cleanDriver: Boolean = false): CompilationInputs = | ||
| CompilationInputs(params.uri().nn, params.text().nn, params.token().nn, cleanDriver) |
There was a problem hiding this comment.
There is already a printed in XtensionVirtualFileParams for VirtualFileParams. It has the benefit of printing also where the cursor / range selection was.
|
|
||
| object CompilationInputs: | ||
| def empty: CompilationInputs = CompilationInputs(new URI(""), "", EmptyCancelToken, false) | ||
| def fromParams(params: VirtualFileParams | OffsetParams, cleanDriver: Boolean = false): CompilationInputs = |
There was a problem hiding this comment.
| def fromParams(params: VirtualFileParams | OffsetParams, cleanDriver: Boolean = false): CompilationInputs = | |
| def fromParams(params: VirtualFileParams, cleanDriver: Boolean = false): CompilationInputs = |
OffsetParams <: VirtualFileParams
| def dequeue(): Option[Task[?]] = Option(queue.poll().nn) | ||
|
|
||
| def shutdownCurrentCompiler(): Unit = | ||
| _driver.set(new CachingDriver(driverSettings)) |
There was a problem hiding this comment.
| _driver.set(new CachingDriver(driverSettings)) | |
| _driver.set(newCompiler()) |
| _driver.set(new CachingDriver(driverSettings)) | ||
|
|
||
| private def getDriver(cleanDriver: Boolean): CachingDriver = | ||
| if cleanDriver then new CachingDriver(driverSettings) |
There was a problem hiding this comment.
| if cleanDriver then new CachingDriver(driverSettings) | |
| if cleanDriver then newCompiler() |
| def scheduleProcessing(): Unit = | ||
| executor.execute(() => processQueue()) |
There was a problem hiding this comment.
So this is called in enqueueTask and it calls processQueue, but processQueue also calls itself. Won't that queue a lot of processQueue calls?
| case _: InterruptedException => executor.shutdownNow() | ||
| handleInfiniteCompilation(executorThread, task) | ||
| () | ||
| }, 60, TimeUnit.SECONDS).nn // refactor and use value from config |
There was a problem hiding this comment.
Possibly should be config.timeoutDelay() + 10 sec?
| logger.log(Level.SEVERE, s"$shortMessage. Full details of the error can be found in the error report $path") | ||
| case _ => | ||
| logger.log(Level.SEVERE, shortMessage + "\n" + stacktrace.indent(2)) | ||
| task.future.completeExceptionally(new InfiniteCompilationException(shortMessage)) |
There was a problem hiding this comment.
So an infinite compilation will cause the whole presentation compiler to shutdown, right? (Before we would take a new compiler and process other tasks in the queue). I think we should somehow reject other tasks that were in the queue and any other incoming tasks.
| finally | ||
| _currentTask.getAndSet(None).nn.foreach(_.interrupt()) |
This PR implements compiler cancellation for PC.
This makes all requests way faster as we will be able to cancel during type-checking.
Because of that, I had to implement new compiler access because it is easier to include it here than modify and depend on external
mtags-sharedcode. The reason is that cancellation throws random errors in the compiler, and by wrapping them in another exception we can at least ignore them in other tools without any problems.The new compiler access also fixes another problem that we were using LIFO queue for tasks, which is incorrect as LSP requires tasks to be return in order they came in.
Lastly, there is new check created to handle infinite compilation.
Before this, PR was silently discarding zombie threads.
I strongly believe that we should instead follow fail fast behaviour. Especially, we should never hide such infinite compilations because it results in Thread starvation, e.g. in Scastie as it is also consuming Presentation Compilers.
Ideally, we would want to throw new
InfiniteCompilationErrorthat should be defined inmtags-interfaces. That will allow consumers of presentation compiler to detect when such case has happened and act accordingly (I recommendSystem.exit). Noone wants to have thread consuming 100% CPU usage in infinite recursionThis is overall new approach that I propose to take. Instead of handling unexpected stuff as we did e.g. in completions when there was no compilation unit present, we should rather throw an error and at least let user know by report that something went wrong.
Feel free to discuss. This PR will stay in draft state until we move
InfiniteCompilationErrorto mtags-interfaces.