Migrate Eclipse-based debuggers to LSP4E

[agarciadom]

After adding the DAP-based debugging, we've ended up with two codebases for debugging: the old Eclipse-based debugger (which is tightly tied to Eclipse APIs and lacks automated tests), and the new DAP-based debugger.

To avoid maintaining two competing codebases, we should migrate the Epsilon debug configurations to use the new DAP-based debugging through LSP4E, and retire the old codebase.

[kolovos]

I've given this a go by changing the behaviour of the "debug" branch of EpsilonLaunchConfigurationDelegates launch method to launch an EpsilonDebugServer followed by an LSP4E debug client as shown below. I've tried this with an EGX/EGL example and the debugger seems to be stopping at the right places, but clicking on tree elements in the Debug view doesn't highlight the respective lines in the EGX/EGL files.

diff --git a/plugins/org.eclipse.epsilon.eol.dt/META-INF/MANIFEST.MF b/plugins/org.eclipse.epsilon.eol.dt/META-INF/MANIFEST.MF
index 62555d0..1639d3e 100644
--- a/plugins/org.eclipse.epsilon.eol.dt/META-INF/MANIFEST.MF
+++ b/plugins/org.eclipse.epsilon.eol.dt/META-INF/MANIFEST.MF
@@ -9,7 +9,12 @@
  org.eclipse.ui.console,
  org.eclipse.ui.workbench.texteditor,
  org.eclipse.ui.ide,
- org.eclipse.epsilon.common.dt
+ org.eclipse.epsilon.common.dt,
+ org.eclipse.lsp4e.debug,
+ org.eclipse.lsp4j.jsonrpc,
+ org.eclipse.lsp4j.jsonrpc.debug,
+ org.eclipse.lsp4j.debug,
+ org.eclipse.epsilon.eol.dap
 Bundle-ActivationPolicy: lazy
 Export-Package: org.eclipse.epsilon.eol.dt,
  org.eclipse.epsilon.eol.dt.debug,
diff --git a/plugins/org.eclipse.epsilon.eol.dt/src/org/eclipse/epsilon/eol/dt/launching/EpsilonLaunchConfigurationDelegate.java b/plugins/org.eclipse.epsilon.eol.dt/src/org/eclipse/epsilon/eol/dt/launching/EpsilonLaunchConfigurationDelegate.java
index cc9e3f0..d8acbac 100644
--- a/plugins/org.eclipse.epsilon.eol.dt/src/org/eclipse/epsilon/eol/dt/launching/EpsilonLaunchConfigurationDelegate.java
+++ b/plugins/org.eclipse.epsilon.eol.dt/src/org/eclipse/epsilon/eol/dt/launching/EpsilonLaunchConfigurationDelegate.java
@@ -11,8 +11,11 @@

 import java.io.File;
 import java.util.ArrayList;
+import java.util.HashMap;
 import java.util.Map;
 import java.util.Set;
+import java.util.Timer;
+import java.util.TimerTask;

 import org.eclipse.core.resources.IFile;
 import org.eclipse.core.resources.IResource;
@@ -37,10 +40,17 @@
 import org.eclipse.epsilon.common.module.IModule;
 import org.eclipse.epsilon.common.parse.problem.ParseProblem;
 import org.eclipse.epsilon.eol.IEolModule;
+import org.eclipse.epsilon.eol.dap.EpsilonDebugServer;
 import org.eclipse.epsilon.eol.debug.EolDebugger;
 import org.eclipse.epsilon.eol.dt.debug.EolDebugTarget;
 import org.eclipse.epsilon.eol.exceptions.EolRuntimeException;
 import org.eclipse.epsilon.eol.execute.context.IEolContext;
+import org.eclipse.lsp4e.debug.debugmodel.DSPDebugTarget;
+import org.eclipse.lsp4e.debug.launcher.DSPLaunchDelegate;
+import org.eclipse.lsp4e.debug.launcher.DSPLaunchDelegate.DSPLaunchDelegateLaunchBuilder;
+import org.eclipse.lsp4j.debug.services.IDebugProtocolServer;
+import org.eclipse.lsp4j.jsonrpc.Launcher;
+import org.eclipse.lsp4j.jsonrpc.MessageConsumer;

 public abstract class EpsilonLaunchConfigurationDelegate extends LaunchConfigurationDelegate implements EpsilonLaunchConfigurationDelegateListener {

@@ -115,12 +125,30 @@
                for (Map.Entry<?, ?> entry : configuration.getAttributes().entrySet()) {
                    launch.setAttribute(entry.getKey() + "", entry.getValue() + "");
                }
-
-               final String name = launch.getAttribute(lauchConfigurationSourceAttribute);
-               target = new EolDebugTarget(launch, module, debugger, name);
-               debugger.setTarget(target);
-               launch.addDebugTarget(target);
-               result = target.debug();
+               
+               // Schedule the debug client to start in a second
+               // to give the debug server enough time to start running
+               new Timer().schedule(new TimerTask() {
+                   
+                   @Override
+                   public void run() {
+                       final DSPLaunchDelegateLaunchBuilder builder = new DSPLaunchDelegateLaunchBuilder(configuration, mode, launch, progressMonitor);
+                       builder.setAttachDebugAdapter("127.0.0.1", 4040);
+                       HashMap<String, Object> parameters = new HashMap<String, Object>();
+                       parameters.put("request", "attach");
+                       builder.setDspParameters(parameters);
+                       DSPLaunchDelegate delegate = new DSPLaunchDelegate();
+                       try {
+                           delegate.launch(builder);
+                       } catch (CoreException e) {
+                           LogUtil.log(e);
+                       }
+                   }
+               }, 1000);
+               
+               // Start a debug server for the module
+               EpsilonDebugServer debugServer = new EpsilonDebugServer(module, 4040);
+               debugServer.run();
            }

            executed(configuration, mode, launch, progressMonitor, module, result);

[kolovos]

Some thoughts for further down the line:

• When we get this to work it may be useful to add another EpsilonDebugServer constructor that also accepts a callback object (e.g. a Runnable) that it runs as soon as the server is ready. We could then provide a callback object to start the debug client instead of using an arbitrary delay.

[agarciadom]

I've just pushed a new lsp4e-debuggers branch that should address the above issues:

• The launch configurations needed to be given a source locator in order to highlight the relevant lines. I've changed all our launch configuration types to do this.
• The timer-based approach is no longer needed, after adding an onStart hook to EpsilonDebugServer to have a bit of code run in a separate thread

I've tried it with EOL and it works as expected. If we're happy with this, we could move on to removing our custom debug UI code in this branch.

[agarciadom]

On another note - we would also need to do something about the Eclipse-based debugging from Ant tasks. I'm not sure if it's enough to have reworked the EpsilonLaunchConfigurationDelegate.

[kolovos]

When a runtime error occurs in debug mode, it is not reported in the console. For example, running the following program which attempts to print the undefined x variable produces an error message in the console when executed in "run" mode but not when executed in "debug" mode.

x.println();

On a related note, when a runtime error occurs, the launch method of EpsilonLaunchConfigurationDelegate should return false. This return value is used in e.g. EmlLaunchConfigurationDelegate so that the EML program is not executed if the ECL program has failed.

[agarciadom]

Thanks for the catches! The old version of the code relied on whatever Epsilon printed to its own console, but I see that printing out the exception produced by a module was left up to the launch delegate. I can't rely on a launch delegate doing that for VS Code, so instead I've given the responsibility to the debug adapter - if a module completes its execution with an exception, it will print it out to the module's standard error stream (which will then be sent to the DAP client).

The second issue reminded me that users of EpsilonDebugServer should be able to retrieve the result of calling module.execute(), whether it was a regular return value, or an exception. I have added a getResult() method that returns a Future<Object> which can resolve into any of those two. This means we can now replace this:

Object result = module.execute();

With this, which should return the same value and throw the same exceptions if the module crashes:

EpsilonDebugServer server = new EpsilonDebugServer(module, port);
server.run();
Object result = server.getResult().get();

[agarciadom]

I have just double checked, and EclipseHost (used for running Ant tasks from inside the Eclipse JVM) still uses the old debuggers (it hooks directly to them). I'll change it to start the server and then use LSP4E debug launch delegate, much like our current Epsilon launch delegate in "debug" mode.

[agarciadom]

I've pushed changes to lsp4e-debuggers so that the EclipseHost creates and launches an LSP4E launch configuration on the fly, based on the port assigned to the EpsilonDebugServer (which supports leaving debugPort unset to use any available port).

Dimitris: with this, we cover both debugging from an Epsilon launch configuration, and from an Ant launch. Shall I start deleting the old code we had for debugging, or am I missing something?

[kolovos]

Thanks for all your hard work! Everything I've tried so far works well except for the following:

• Running the stored launch configuration in the attached project in "debug" mode, doesn't throw the expected exception.
• Running a simple EOL program with an error e.g. x.println(); in "debug" mode produces an exception in the console but after a few seconds, when the launch configuration finishes, the console is cleared and the exception text disappears.

[agarciadom]

Thanks the the report!

I'll investigate the first item ASAP and let you know what I find.

For the second behaviour, it seems like a case of "it's a feature, not a bug". When our EpsilonLaunchConfigurationDelegate notices that a run/debug configuration crashes with an exception, it calls progressMonitor.setCanceled(true). Ths is understood by the debug UI in Eclipse as wanting to remove the launch, which triggers the ProcessConsoleManager in org.eclipse.debug.internal.ui to remove the console for it as well.

The odd thing is that the same cancellation behaviour doesn't do anything when the program crashes from "run" mode. This appears to be because the launch in this case does not have any IProcess objects, unlike in the debug mode where the DAP side of LSP4E will add one for us. I wonder if this difference in behaviour may be a reason for the odd behaviour in #104: maybe recent versions of Eclipse require having an IProcess associated to a running program?

Would it make sense to just change the behaviour so crashing out of an Epsilon program won't consider the execution to be cancelled (it did complete, it just didn't complete successfully)?

[agarciadom]

I've pushed a change removing the call to progressMonitor.setCanceled(true) and the terminal stays open with its output after the launch completes.

[agarciadom]

The first item was due to a NullPointerException in EpsilonDebugAdapter#sendOutput while trying to send out the error line, as it was relying on the frame having set a current statement and it didn't have the fallback to entrypoint + module that the stackTrace() method had. I've unified the way in which we resolve the current location across both places, and now I see the same error being reported in the ECL+EML launch configuration you provided.

[agarciadom]

I have fixed the debugging of EML launch configurations from LSP4E: it is now possible to stop on breakpoints both in the ECL and the EML scripts. I had to use a "combo module" approach, as the old approach of running the ECL delegate and then the regular EML delegate didn't work well with LSP4E.

I have also noted that in this case, finishing the execution of the program in "debug" mode may sometimes leave "hanging" threads despite our DAP server having sent the "thread ended" event, as you reported before. From what I see, this appears to be a timing issue, and a limitation in how LSP4E processes the terminated event, where it will immediately shut down the debugging session without processing the exited event that indicates a successful execution of the program:

https://github.com/eclipse/lsp4e/issues/266

When I have tried setting a breakpoint before we send the terminated message (to force LSP4E to process the thread events before we send the terminated event), it has worked fine. This sounds like it may be an LSP4E issue and not on our side.