Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get server back into working state #107

Merged
merged 6 commits into from
Dec 10, 2017
Merged

Conversation

olafurpg
Copy link
Member

@olafurpg olafurpg commented Dec 9, 2017

The combination of #104 #103 and #100 made the server almost unusable. This PR tries to repair a bit of the damage by

  • pushing notifications and commands onto the Scheduler instead of main thread. Commands were accidentally running on the main thread because I used Task.now 馃槄 . Notifications were also blocking the main thread because we compile on every change after Produce a stream of semanticdbs as you type聽#100.
  • disable interactive semanticdb Produce a stream of semanticdbs as you type聽#100, scalac and scalafix were racing to publish diagnostics making red squigglies jitter in the editor. I propose we dedicate a PR to fix that issue, this PR is only about returning the server to a functioning state for people who checkout master.
  • reduce the logging noise to the clients from Move all requests to Task[T]聽#103 by skipping Message{Reader/Writer} entries. Maybe there's a better way to implement that filter in logback.groovy, but my logback/groovy foo is not great

The server was freezing because it was running compile on the main
thread.
- Handle previously uncaught Initialized request from client
- Remove unused text document manager, we use Buffers instead.
Previously, we used Task.now to eagerly compute all values. This blocked
up the server immediately and made the server fail to handle requests
like shutdown.

This commit also reduces noise in the logs, which made it easier to
debug what was happening.  Previously we logged everything messages to
the client, including the fact that we were logging to the client. This
was a lot of noise and made it difficult to see what was happening.
@olafurpg
Copy link
Member Author

olafurpg commented Dec 9, 2017

The PC is still getting stuck in an infinite loop

   java.lang.Thread.State: RUNNABLE
	at scala.collection.TraversableLike$WithFilter.foreach(TraversableLike.scala:788)
	at scala.tools.nsc.interactive.Global$Members.addNonShadowed(Global.scala:1006)
	at scala.tools.nsc.interactive.Global.localsToEnclosing$1(Global.scala:1030)
	at scala.tools.nsc.interactive.Global.scopeMembers(Global.scala:1043)
	at scala.tools.nsc.interactive.Global.completionsAt(Global.scala:1242)
	at scala.meta.languageserver.compiler.CompilerUtils$.safeCompletionsAt(CompilerUtils.scala:13)
	at scala.meta.languageserver.providers.CompletionProvider$.completions(CompletionProvider.scala:77)
	at scala.meta.languageserver.ScalametaLanguageServer.$anonfun$completion$1(ScalametaLanguageServer.scala:183)
	at scala.meta.languageserver.ScalametaLanguageServer$$Lambda$1726/167989916.apply(Unknown Source)
	at monix.eval.internal.TaskRunLoop$.monix$eval$internal$TaskRunLoop$$loop$1(TaskRunLoop.scala:187)
	at monix.eval.internal.TaskRunLoop$RestartCallback$1.onSuccess(TaskRunLoop.scala:119)
	at monix.eval.Task$.$anonfun$forkedUnit$2(Task.scala:1463)
	at monix.eval.Task$$$Lambda$903/353321401.run(Unknown Source)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
	at java.lang.Thread.run(Thread.java:745)

I have two helpful scripts to debug it

bash-3.2$ cat lsp_kill
jps | grep coursier | awk '{ print $1 }' | xargs kill
bash-3.2$ cat lsp_stack
jps | grep coursier | awk '{ print $1 }' | xargs jstack | grep RUNNABLE -A 20

lsp_kill is especially helpful to kill a zombie process when the shutdown procedure fails to run.

Copy link
Member

@gabro gabro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

馃憤 thanks for taking this on! I've left two non blocking comments and I'll add the logging configuration change directly on top of this branch.

else event.getFormattedMessage
connection.foreach(_.logMessage(MessageType.Log, message))
// Skip rpc message noise.
if (!event.getLoggerName.startsWith("langserver.core.Message")) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be configured moreflexibly in logback.groovy. I'll push this change directly on the PR.

@@ -325,21 +318,6 @@ object ScalametaLanguageServer extends LazyLogging {
subscriber -> semanticdbPublisher
}

def interactiveSemanticdbStream(cwd: AbsolutePath)(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you remove this entirely because you think there's a better approach? Otherwise just commenting/removing the code that invokes this should be enough.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think There may be a better way to model this as a stream from the notifications, want to experiment with it today

@@ -13,3 +13,5 @@ appender("LSP", scala.meta.languageserver.LSPLogger) {
}

root(DEBUG, ["LSP", "STDOUT"])
logger("langserver.core.MessageWriter", INFO, ["LSP", "STDOUT"])
logger("langserver.core.MessageReader", INFO, ["LSP", "STDOUT"])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@olafurpg this should be easier to tweak. I've opted for raising the level to INFO, since the noisy RPC messages have a DEBUG level.

@olafurpg olafurpg merged commit ebe0c35 into scalameta:master Dec 10, 2017
@olafurpg olafurpg deleted the cleanup branch December 10, 2017 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants