Implement support for the Debug Adapter Protocol by mzarnowski · Pull Request #944 · scalacenter/bloop

mzarnowski · 2019-06-26T13:57:07Z

No description provided.

tgodzik

Added some preliminary comments - let me know when you want the full review

frontend/src/main/scala/bloop/dap/JavaDebugInterface.scala

frontend/src/main/scala/bloop/exec/JvmProcessForker.scala

This prevents the files being served in non-deterministic order

This commit is the result of a pair-programming session between me and Marek where we went through some of the previous abstractions and simplified them. Now, there are only two core abstractions `DebugServer` and a `DebugSession`. Everything else reuses the machinery created by `run`. Co-authored-by: Marek Żarnowski <mzarnowski@virtuslab.com>

`ConnectionHandle` is not a handle when it holds the state of the connection. Therefore, this commit removes any state from it so that the client is responsible for opening and closing the socket. This simplifies reasoning about what does `ConnectionHandle` do. Furthermore, as connection handle is too generic of a name, we settle on the `ServerHandle` name which makes it more clear that this handle is mapped to a server instead of a client connection.

The listening logic before was a while loop that lacked any communication with the current spawned session. Since most of the times there should only be one open connection and we cannot process more while that one is running, there is no use in having an independent task blocking on `serverSocket.accept()`. Instead, what we do is that we only have one task listening and then serving a client. When the DAP client signals that it's interested in restarting the session, then we recurse (safely, because of Task's trampolining) and start accepting connections again.

…en bloop plugin

…ient

jvican

Excellent work @marek1840 🔥 💯 I'm really excited with the current state of the implementation, it's obvious you have put a lot of effort into implementing this and improving it from version to version. I don't have any more feedback to add aside from some comments on testing that can be addressed in a follow-up PR.

I'm going to be merging this as soon as the bsp artifacts hit Maven Central and CI passes. 🎉 Thanks a lot for this! Let's schedule a discussion tomorrow to discuss over some of the suggestions I leave in my comments.

jvican · 2019-08-01T20:57:59Z

frontend/src/test/scala/bloop/dap/DebugProtocolSpec.scala

+
+        val lines = output.lines.toSeq
+        assert(lines.size == 2)
+        assert(lines(0).contains("Running Tests"))


Could we use assertNoDiff here instead of manually asserting?

Also, I don't understand this test. Here's a few questions:

Why is our ouput only showing test output for one test suite?

Why are we passing the test filters?

Are those filters acting as some kind of blacklist? If so, why? The semantics of test filters is that they specify test suites that must be included in the test execution, effectively acting as a whitelist, this seems contradictory from what I read in the test suite.

Why does removing those filters cause the test to deadlock?

because of a bug and my ill understanding of what happened ;)

Now, we use them to verify the output

those are exactly the same kind of filters as expected by the buildTarget/test endpoints

I am not sure, I could not reproduce. Can you verify it still happens with the newest version?

jvican · 2019-08-01T21:04:47Z

frontend/src/test/scala/bloop/dap/DebugProtocolSpec.scala

+      }
+    }
+  }
+


Can we add a test when we restart the execution of a test suite while the program is still running tests suites? Maybe also think of other test cases we might want to be testing? Like what happens when things go wrong?

DebugServerSpec.restarting closes current client and debuggee should verify that both the connection and the debuggee (a task obtained from TestTask.runTestSuites) are cancelled.
It's true, I have not verified if this task cancels correctly - I will look into it soon.

in short, I was thinking of DebugServerSpec as a test suite which abstracts over the debuggee and verifies the server itself - something closer to unit tests than integration tests. DebugProtocolSpec is used to verify that the actual protocol works correctly - those are the "real" integration tests.

jvican · 2019-08-01T21:11:33Z

frontend/src/test/scala/bloop/dap/DebugProtocolSpec.scala

+    }
+  }
+
+  test("picks up source changes across sessions") {


While thinking about more scenarios for our test cases, I've realized that we're only picking up source changes across sessions. So, a restart action from vscode will not pick up any changes to the sources, it will instead just force a new start of the VM with the same class files. Let's have a call tomorrow about how we can work around this.

here is a small PoC for recompiling sources before starting the debuggee: https://github.com/marek1840/bloop/pull/4

mzarnowski

Thanks for the review and making this branch up to date!
There is one concern about the user-facing messages in about.

mzarnowski · 2019-08-02T07:39:35Z

frontend/src/main/scala/bloop/engine/Interpreter.scala

-    val jdiStatus = if (JavaEnv.resolveDebugInterface.isSuccess) "available" else "unavailable"
-    logger.info(s"Java Debug interface is $jdiStatus")
+    logger.info(s"Running on Java v$javaVersion ($javaHome)")
+    logger.info(s"Detected Java ${JavaEnv.detectRuntime} runtime $jdiStatus")


Since the message in Feedback explicitly mentions Java Debug Interface, I think we should mention it here as well so we can avoid the confusion.
Also "Detected Java JRE runtime ..." sounds a bit off when dropped the abbreviation:
Detected Java Java Runtime Environment runtime ...

maybe we could display the JDK/JRE info with the java version and leave this line for JDI exclusively?

mzarnowski · 2019-08-02T07:41:00Z

frontend/src/main/scala/bloop/engine/Interpreter.scala

+    val jdiStatus =
+      if (JavaEnv.loadJavaDebugInterface.isSuccess) "supports debugging"
+      else "doesn't support debugging!"
+    val runtimeInfo = s"Detected Java ${JavaEnv.detectRuntime} runtime $jdiStatus"


not used

Suggested change

val runtimeInfo = s"Detected Java ${JavaEnv.detectRuntime} runtime $jdiStatus"

It seems this event might not be emitted when we disconnect from the JVM

Previously the response might not have been sent, because of a race condition between: 1. asynchronously sending a response to the client 2. receiving a terminated event from the debuggee, finishing communication done promise and, as a result, triggering the shutdown sequence which closes the socket

jvican · 2019-09-14T19:55:33Z

@marek1840 I think we're ready to merge this! Thanks for all of your hard work. 👍

I have added a documentation page in our website talking about the DAP <> BSP implementation in Bloop. I have added a limitations section that explains what users might expect from this integration and current gotchas.

Given that there are some important things missing in our current debug implementation, I'll omit this feature in 1.3.3 release notes. Between 1.3.3 and 1.4.0, we can work on:

Adding breakpoint support.
Adding a BSP notification to return the response produced by the BSP action run during the DAP session. This is the first limitation I list in the documentation page; there is currently no way that bloop can send TestResult or RunResult to the BSP client.

I'll take care of adding breakpoint support, I want to get more familiar with the debugging JDI APIs and make sure I understand them well enough to maintain them during the next years.

@marek1840 Could you look at the second item? We should have a chat to discuss what the best way to implement that is because there are several solutions. After that, I propose you work on the debugging UX side of things in Metals as well as the unified debug/test/run interface. That's where most of the impact is to be made.

Let me know what your thoughts are when you come back from vacation! 😄

It looks the previous logic would fail in in certain Windows systems where the Windows Search is disabled, with a similar error to this one: ``` java.lang.IllegalStateException: Missing java executable at C:\Program Files\Java\jdk1.8.0_202\jre\bin\java! bloop.exec.JvmForker.javaExecutable(JvmProcessForker.scala:133) bloop.exec.JvmForker.runMain(JvmProcessForker.scala:114) bloop.exec.JvmProcessForker.runMain(JvmProcessForker.scala:56) bloop.exec.JvmProcessForker.runMain$(JvmProcessForker.scala:44) bloop.exec.JvmForker.runMain(JvmProcessForker.scala:90) ``` This bug intends to avoid throwing that exception by resolving `java.exe` instead of just `java`.

jvican · 2019-09-19T08:54:52Z

One limitation I forgot to add to our docs is that restarting a session via DAP doesn't trigger compilation of the involved projects. This is an intentional design decision.

BSP clients can decide how to handle this particular scenario. For clients such as Metals that are proxying DAP communication from the BSP server to the end client (VS Code), this limitation can be avoided by intercepting the DAP restart request from VS Code, closing the current DAP session, triggering a compile and then starting a new DAP session. This is the ideal way to solve this problem because Metals will be aware there's a compilation going on and will correctly report diagnostics to the user as well as results and timings. If restarting a DAP session implied recompiling a project, its implementation in bloop and Metals would be more complicated. It's better to make BSP compile requests explicit in the protocol and not rely on action side effects of the DAP request.

olafurpg · 2019-09-19T09:31:46Z

@jvican Good to know, thanks for sharing!

mzarnowski

I have noticed some discrepancies, mostly in the docs.

mzarnowski · 2019-09-20T09:49:35Z

frontend/src/main/scala/bloop/dap/DebugSession.scala

        } finally {
-          // "exited" event may not be sent if the debugger disconnects from the jvm beforehand
-          remainingTerminalEvents.remove("exited")
+          // Exited event should not be expected if debugger has already disconnected from the JVM


this comment tells "what" and not "why", and "what" can be read in the expectedTerminalEvents.remove("exited") statement. In my experience it is better to explain - at least partially - why we are doing something instead of repeating what we are doing, but using different words.

mzarnowski · 2019-09-20T09:52:01Z

frontend/src/main/scala/bloop/dap/DebugSessionLogger.scala

    listener: InetSocketAddress => Unit,
-    underlying: Logger
+    underlying: Logger,
+    initialized0: Option[Atomic[Boolean]] = None


if we make it Atomic[Boolean] = Atomic(false) then the field initialized is unnecessary and this parameter can lose the 0 suffix

mzarnowski · 2019-09-20T09:53:43Z

docs/debugging-reference.md

+sidebar_label: Debugging Reference
+---
+
+Bloop allows [BSP][bsp] build clients to start a [DAP][dap] (Debugging


bsp build clients == build server protocol build client. we can drop the second build

mzarnowski · 2019-09-20T09:54:10Z

docs/debugging-reference.md

+sidebar_label: Debugging Reference
+---
+
+Bloop allows [BSP][bsp] build clients to start a [DAP][dap] (Debugging


DAP stands for Debug Adapter Protocol, not Debugging Adapter Protocol

mzarnowski · 2019-09-20T09:54:42Z

docs/debugging-reference.md

+Bloop allows [BSP][bsp] build clients to start a [DAP][dap] (Debugging
+Adapter Protocol) session to run a project or its tests. The core of the
+debugging implementation in Bloop relies on Microsoft's [java-debug][]
+library and it's exposed through [DAP][dap] support in [BSP][bsp].


it's -> is

mzarnowski · 2019-09-20T12:14:31Z

docs/debugging-reference.md

+  def targets: List[BuildTargetIdentifier]
+
+  /** The kind of data to expect in the `data` field. Mandatory. */
+  def dataKind: Option[String]


If it is Mandatory, then it should be of type String, not Option[String]

mzarnowski · 2019-09-20T12:14:47Z

docs/debugging-reference.md

+  def dataKind: Option[String]
+
+  /** A language-agnostic JSON object interpreted by the server. Mandatory. */
+  def data: Option[Json]


If it is Mandatory, then it should be of type String, not Option[String]

mzarnowski · 2019-09-20T12:31:56Z

docs/debugging-reference.md

+The data field contains language-agnostic information about what action
+should the server debug. Bloop supports only two data kinds:
+
+1. `scala-main-class` when `data` contains [`ScalaRunParams`]( https://github.com/scalacenter/bsp/blob/master/docs/bsp.md#scala-run-params).


the link leads nowhere.
Also, see the implementation:
scala-main-class corresponds to ScalaMainClass, not ScalaRunParams.
scala-test-suites corresponds to a list of strings (test suites), not ScalaTestParams

jvican mentioned this pull request Jun 27, 2019

Describe the DAP integration #904

Closed

jvican changed the title ~~Dap integration~~ Implement the Debug Adapter Protocol Jun 27, 2019

jvican added build server Any issue or pull request that has to do with hot compilers or BSP. feature task / debug labels Jun 27, 2019

tgodzik reviewed Jun 28, 2019

View reviewed changes

frontend/src/main/scala/bloop/dap/JavaDebugInterface.scala Outdated Show resolved Hide resolved

frontend/src/main/scala/bloop/dap/JavaDebugInterface.scala Outdated Show resolved Hide resolved

frontend/src/main/scala/bloop/exec/JvmProcessForker.scala Outdated Show resolved Hide resolved

mzarnowski and others added 24 commits June 29, 2019 19:41

Describe the DAP integration with metals example

d89f11a

Describe DAP extension to the BSP

8c5b5d4

Extract connection handle

c509336

simplify mapping targets to projects

ce86989

sort directory snapshot in tests

de46787

This prevents the files being served in non-deterministic order

expose debugSession/start endpoint to client

040b980

connect to Debug Adapter

0b7f0b3

set up Java Debug Server

320d5b7

Add a few renames and remove dead code

118b1a2

Upgrade to latest bsp version

5b0f7f8

Add debug information about configuration to be serialized in the mva…

37a5a67

…en bloop plugin

Fix link reference in docs

f9dbf29

Update export.md

4949181

set up Java Debug Server

9630702

close the Debug Session

998953d

handle the debuggee lifecycle inside the debug session

8e731e1

update DebugSession comment

b609005

verify the server socket is being closed

969a7c0

verify failing debuggee does not affect the communication with the cl…

2aad60f

…ient

force correct backlog for the debug server

3cdae2d

verify source changes are picked up between debug sessions

238ad34

jvican and others added 6 commits August 1, 2019 20:38

Remove warnings

0cb9b22

verify that tests can be run using DAP

fdb2b4e

fail fast when java executable does not exist

89a59f3

Improve missing java executable feedback

ec6ab36

Add last stylistic improvements

bea4d93

Merge branch 'master' into dap-integration

c511bbb

jvican approved these changes Aug 1, 2019

View reviewed changes

mzarnowski commented Aug 2, 2019

View reviewed changes

Marek Żarnowski added 3 commits August 2, 2019 13:09

run tests from many projects using same runner

adc9fa8

disconnect from the client when done

c64acb3

don't wait for exited event when disconnected

e7771c6

It seems this event might not be emitted when we disconnect from the JVM

mzarnowski mentioned this pull request Sep 6, 2019

notify BSP client the debuggee can be attached to #1027

Closed

Marek Żarnowski added 3 commits September 11, 2019 10:33

simplify terminating

715ed00

use bsp version with simplified DebugSessionParams

3177e0c

mzarnowski mentioned this pull request Sep 12, 2019

Add DAP capabilities to BSP build-server-protocol/build-server-protocol#99

Merged

mzarnowski requested a review from jvican September 13, 2019 07:49

jvican added 3 commits September 13, 2019 13:27

Merge branch 'master' into dap-integration

7225474

Merge with master, upgrade bsp and add style changes

a7231f7

Add complete DAP documentation page [DOCS]

28b2105

jvican approved these changes Sep 14, 2019

View reviewed changes

jvican added 2 commits September 15, 2019 00:47

Address feedback about JDI log in bloop about

1da5a79

jvican merged commit 707b2c2 into scalacenter:master Sep 14, 2019

jvican changed the title ~~Implement the Debug Adapter Protocol~~ Implement support for the Debug Adapter Protocol Sep 19, 2019

mzarnowski commented Sep 20, 2019

View reviewed changes

mzarnowski mentioned this pull request Oct 30, 2019

Make forker aware of JDI hardcoded '\n' message #1072

Closed

Conversation

mzarnowski commented Jun 26, 2019

Uh oh!

tgodzik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jvican left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mzarnowski Aug 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mzarnowski left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jvican commented Sep 14, 2019

Uh oh!

jvican commented Sep 19, 2019

Uh oh!

olafurpg commented Sep 19, 2019

Uh oh!

mzarnowski left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mzarnowski Aug 2, 2019 •

edited

Loading

mzarnowski left a comment •

edited

Loading