Skip to content

Commit cd2c369

Browse files
sebthomsebthom
authored
fix(debug): defer adapter shutdown until exited event (#1402)
1 parent e32b8a6 commit cd2c369

3 files changed

Lines changed: 322 additions & 11 deletions

File tree

‎org.eclipse.lsp4e.debug/src/org/eclipse/lsp4e/debug/console/DSPProcess.java‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,14 +65,24 @@ public boolean isTerminated() {
6565
return handle.map(h -> !h.isAlive()).orElse(terminated);
6666
}
6767

68-
@Override
69-
public void terminate() throws DebugException {
68+
/**
69+
* Terminates the OS process and notifies the debug framework without issuing
70+
* any additional protocol requests to the debug adapter. This is used when the
71+
* adapter has already signalled that the debuggee has ended (for example via
72+
* 'terminated'/'exited' events).
73+
*/
74+
public void terminateWithoutProtocolRequest() {
7075
terminated = true;
7176
handle.ifPresent(h -> {
7277
h.destroy(); // normal termination
7378
CompletableFuture.runAsync(h::destroyForcibly, CompletableFuture.delayedExecutor(5, TimeUnit.SECONDS)); // forced termination if normal is not sufficient
7479
});
7580
DebugPlugin.getDefault().fireDebugEventSet(new DebugEvent[] { new DebugEvent(this, DebugEvent.TERMINATE) });
81+
}
82+
83+
@Override
84+
public void terminate() throws DebugException {
85+
terminateWithoutProtocolRequest();
7686
target.terminate();
7787
}
7888

‎org.eclipse.lsp4e.debug/src/org/eclipse/lsp4e/debug/debugmodel/DSPDebugTarget.java‎

Lines changed: 25 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
import java.util.concurrent.ExecutorService;
3636
import java.util.concurrent.Executors;
3737
import java.util.concurrent.Future;
38+
import java.util.concurrent.TimeUnit;
3839
import java.util.function.Predicate;
3940
import java.util.function.Supplier;
4041
import java.util.function.UnaryOperator;
@@ -133,8 +134,9 @@ public class DSPDebugTarget extends DSPDebugElement implements IDebugTarget, IDe
133134
*/
134135
private final Map<Integer, DSPThread> threads = Collections.synchronizedMap(new TreeMap<>());
135136

136-
private boolean fTerminated = false;
137-
private boolean fSentTerminateRequest = false;
137+
private volatile boolean exitedReceived = false;
138+
private volatile boolean fTerminated = false;
139+
private volatile boolean fSentTerminateRequest = false;
138140
private String targetName = lateNonNull();
139141

140142
private @Nullable DSPBreakpointManager breakpointManager;
@@ -319,11 +321,7 @@ private void terminated() {
319321
}
320322
final var process = this.process;
321323
if (process != null && process.canTerminate()) {
322-
try {
323-
process.terminate();
324-
} catch (DebugException e) {
325-
DSPPlugin.logError(e);
326-
}
324+
process.terminateWithoutProtocolRequest();
327325
}
328326
fireTerminateEvent();
329327
debuggees.forEach(DSPDebugTarget::terminated);
@@ -404,7 +402,19 @@ public boolean isTerminated() {
404402

405403
@Override
406404
public void terminated(TerminatedEventArguments body) {
407-
terminated();
405+
/*
406+
* According to the Debug Adapter Protocol, the 'terminated' event signals that
407+
* the debuggee has finished, but the debug adapter may continue to run in
408+
* order to shut down cleanly and emit a corresponding 'exited' event. Do not
409+
* tear down the adapter connection immediately here so that a subsequent
410+
* 'exited' event can still be delivered. If no 'exited' event is received
411+
* within a grace period, fall back to terminating the session.
412+
*/
413+
CompletableFuture.runAsync(() -> {
414+
if (!exitedReceived && !isTerminated()) {
415+
terminated();
416+
}
417+
}, CompletableFuture.delayedExecutor(2, TimeUnit.SECONDS, threadPool));
408418
}
409419

410420
/**
@@ -589,7 +599,13 @@ public boolean supportsBreakpoint(IBreakpoint breakpoint) {
589599

590600
@Override
591601
public void exited(ExitedEventArguments args) {
592-
// TODO
602+
/*
603+
* The debug adapter reports that the debuggee has exited. At this point it is
604+
* safe to terminate the debug session and dispose the connection to the
605+
* adapter.
606+
*/
607+
exitedReceived = true;
608+
terminated();
593609
}
594610

595611
@Override

‎org.eclipse.lsp4e.test/src/org/eclipse/lsp4e/test/debug/DebugSessionTerminationTest.java‎

Lines changed: 285 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,285 @@
1+
/*******************************************************************************
2+
* Copyright (c) 2025 Vegard IT GmbH and others.
3+
* This program and the accompanying materials are made
4+
* available under the terms of the Eclipse Public License 2.0
5+
* which is available at https://www.eclipse.org/legal/epl-2.0/
6+
*
7+
* SPDX-License-Identifier: EPL-2.0
8+
*
9+
* Contributors:
10+
* Sebastian Thomschke (Vegard IT GmbH) - initial implementation.
11+
*******************************************************************************/
12+
package org.eclipse.lsp4e.test.debug;
13+
14+
import static org.junit.jupiter.api.Assertions.*;
15+
16+
import java.io.InputStream;
17+
import java.io.OutputStream;
18+
import java.util.HashMap;
19+
import java.util.Map;
20+
import java.util.concurrent.CompletableFuture;
21+
import java.util.concurrent.ExecutorService;
22+
import java.util.concurrent.atomic.AtomicBoolean;
23+
import java.util.concurrent.atomic.AtomicInteger;
24+
import java.util.function.UnaryOperator;
25+
26+
import org.eclipse.core.runtime.NullProgressMonitor;
27+
import org.eclipse.debug.core.DebugPlugin;
28+
import org.eclipse.debug.core.ILaunch;
29+
import org.eclipse.debug.core.ILaunchConfigurationType;
30+
import org.eclipse.debug.core.ILaunchConfigurationWorkingCopy;
31+
import org.eclipse.debug.core.ILaunchManager;
32+
import org.eclipse.debug.core.Launch;
33+
import org.eclipse.lsp4e.debug.debugmodel.DSPDebugTarget;
34+
import org.eclipse.lsp4e.debug.debugmodel.TransportStreams;
35+
import org.eclipse.lsp4e.test.utils.AbstractTestWithProject;
36+
import org.eclipse.lsp4e.test.utils.TestUtils;
37+
import org.eclipse.lsp4j.debug.Capabilities;
38+
import org.eclipse.lsp4j.debug.DisconnectArguments;
39+
import org.eclipse.lsp4j.debug.ExitedEventArguments;
40+
import org.eclipse.lsp4j.debug.InitializeRequestArguments;
41+
import org.eclipse.lsp4j.debug.ProcessEventArguments;
42+
import org.eclipse.lsp4j.debug.TerminateArguments;
43+
import org.eclipse.lsp4j.debug.TerminatedEventArguments;
44+
import org.eclipse.lsp4j.debug.services.IDebugProtocolClient;
45+
import org.eclipse.lsp4j.debug.services.IDebugProtocolServer;
46+
import org.eclipse.lsp4j.jsonrpc.Launcher;
47+
import org.eclipse.lsp4j.jsonrpc.MessageConsumer;
48+
import org.eclipse.lsp4j.jsonrpc.RemoteEndpoint;
49+
import org.junit.jupiter.api.Test;
50+
51+
/**
52+
* Regression test for https://github.com/eclipse-lsp4e/lsp4e/issues/266: ensure
53+
* LSP4E.debug does not terminate the debug adapter too early.
54+
*
55+
* Scenario:
56+
* <ol>
57+
* <li>create a mock DAP server and wire it into a DSPDebugTarget via a Launcher
58+
* stub (no real IO)
59+
* <li>server sends a ProcessEvent so LSP4E creates a DSPProcess
60+
* <li>server sends a terminated event followed by an exited event
61+
* <li>assert that the exited event can still be delivered without the
62+
* connection being torn down
63+
* </ol>
64+
*/
65+
class DebugSessionTerminationTest extends AbstractTestWithProject {
66+
67+
/**
68+
* Minimal in-memory mock of a DAP server sufficient for this test.
69+
*/
70+
private static final class MockDebugServer implements IDebugProtocolServer {
71+
IDebugProtocolClient client;
72+
final AtomicBoolean exitedDelivered = new AtomicBoolean(false);
73+
final AtomicInteger terminateRequestCount = new AtomicInteger(0);
74+
final AtomicInteger disconnectRequestCount = new AtomicInteger(0);
75+
76+
@Override
77+
public CompletableFuture<Capabilities> initialize(InitializeRequestArguments args) {
78+
var caps = new Capabilities();
79+
// Advertise support for terminate so that DSPDebugTarget will send a
80+
// terminate request when it (incorrectly) decides to terminate the adapter.
81+
caps.setSupportsTerminateRequest(true);
82+
if (client != null) {
83+
client.initialized();
84+
}
85+
return CompletableFuture.completedFuture(caps);
86+
}
87+
88+
@Override
89+
public CompletableFuture<Void> launch(Map<String, Object> args) {
90+
if (client != null) {
91+
var process = new ProcessEventArguments();
92+
process.setName("mock-debuggee");
93+
client.process(process);
94+
95+
var terminated = new TerminatedEventArguments();
96+
client.terminated(terminated);
97+
98+
var exited = new ExitedEventArguments();
99+
exited.setExitCode(0);
100+
try {
101+
client.exited(exited);
102+
exitedDelivered.set(true);
103+
} catch (Throwable t) {
104+
exitedDelivered.set(false);
105+
}
106+
}
107+
return CompletableFuture.completedFuture(null);
108+
}
109+
110+
@Override
111+
public CompletableFuture<Void> terminate(TerminateArguments args) {
112+
terminateRequestCount.incrementAndGet();
113+
return CompletableFuture.completedFuture(null);
114+
}
115+
116+
@Override
117+
public CompletableFuture<Void> disconnect(DisconnectArguments args) {
118+
disconnectRequestCount.incrementAndGet();
119+
return CompletableFuture.completedFuture(null);
120+
}
121+
}
122+
123+
/**
124+
* Mock server that only sends a 'terminated' event (no 'exited') to exercise
125+
* the fallback termination path in DSPDebugTarget.
126+
*/
127+
private static final class MockTerminatedOnlyServer implements IDebugProtocolServer {
128+
IDebugProtocolClient client;
129+
final AtomicBoolean terminatedDelivered = new AtomicBoolean(false);
130+
final AtomicInteger terminateRequestCount = new AtomicInteger(0);
131+
final AtomicInteger disconnectRequestCount = new AtomicInteger(0);
132+
133+
@Override
134+
public CompletableFuture<Capabilities> initialize(InitializeRequestArguments args) {
135+
var caps = new Capabilities();
136+
caps.setSupportsTerminateRequest(true);
137+
if (client != null) {
138+
client.initialized();
139+
}
140+
return CompletableFuture.completedFuture(caps);
141+
}
142+
143+
@Override
144+
public CompletableFuture<Void> launch(Map<String, Object> args) {
145+
if (client != null) {
146+
var process = new ProcessEventArguments();
147+
process.setName("mock-debuggee");
148+
client.process(process);
149+
150+
var terminated = new TerminatedEventArguments();
151+
client.terminated(terminated);
152+
terminatedDelivered.set(true);
153+
}
154+
return CompletableFuture.completedFuture(null);
155+
}
156+
157+
@Override
158+
public CompletableFuture<Void> terminate(TerminateArguments args) {
159+
terminateRequestCount.incrementAndGet();
160+
return CompletableFuture.completedFuture(null);
161+
}
162+
163+
@Override
164+
public CompletableFuture<Void> disconnect(DisconnectArguments args) {
165+
disconnectRequestCount.incrementAndGet();
166+
return CompletableFuture.completedFuture(null);
167+
}
168+
}
169+
170+
/**
171+
* DSPDebugTarget variant that injects a mock server without real JSON-RPC IO.
172+
*/
173+
private static final class TestDebugTarget extends DSPDebugTarget {
174+
private final IDebugProtocolServer server;
175+
176+
TestDebugTarget(ILaunch launch, Map<String, Object> dspParameters, IDebugProtocolServer server) {
177+
super(launch, () -> new TransportStreams.DefaultTransportStreams(InputStream.nullInputStream(),
178+
OutputStream.nullOutputStream()), dspParameters);
179+
this.server = server;
180+
}
181+
182+
@Override
183+
protected Launcher<? extends IDebugProtocolServer> createLauncher(UnaryOperator<MessageConsumer> wrapper,
184+
InputStream in, OutputStream out, ExecutorService threadPool) {
185+
if (server instanceof MockDebugServer m) {
186+
m.client = this;
187+
} else if (server instanceof MockTerminatedOnlyServer m) {
188+
m.client = this;
189+
}
190+
return new Launcher<>() {
191+
@Override
192+
public RemoteEndpoint getRemoteEndpoint() {
193+
return null;
194+
}
195+
196+
@Override
197+
public IDebugProtocolServer getRemoteProxy() {
198+
return server;
199+
}
200+
201+
@Override
202+
public CompletableFuture<Void> startListening() {
203+
return CompletableFuture.completedFuture(null);
204+
}
205+
};
206+
}
207+
}
208+
209+
private static final String LAUNCH_TYPE_ID = "org.eclipse.lsp4e.debug.launchType";
210+
211+
private static ILaunch newLaunch(String mode) throws Exception {
212+
ILaunchConfigurationType type = DebugPlugin.getDefault().getLaunchManager()
213+
.getLaunchConfigurationType(LAUNCH_TYPE_ID);
214+
ILaunchConfigurationWorkingCopy wc = type.newInstance(null,
215+
"DebugSessionTerminationTest-" + System.currentTimeMillis());
216+
return new Launch(wc, mode, null);
217+
}
218+
219+
@Test
220+
void testTerminatedDoesNotPreventExited() throws Exception {
221+
ILaunch launch = newLaunch(ILaunchManager.RUN_MODE);
222+
223+
var params = new HashMap<String, Object>();
224+
params.put("type", "mock");
225+
params.put("request", "launch");
226+
params.put("program", "dummy");
227+
228+
var server = new MockDebugServer();
229+
var target = new TestDebugTarget(launch, params, server);
230+
231+
target.initialize(new NullProgressMonitor());
232+
233+
// Wait until the terminated event from the adapter has been processed.
234+
TestUtils.waitForAndAssertCondition(5_000, target::isTerminated);
235+
236+
// Sanity check: the target should be marked as terminated.
237+
assertTrue(target.isTerminated(), "Debug target should be terminated");
238+
239+
// The exited event should still be deliverable after terminated.
240+
assertTrue(server.exitedDelivered.get(), "Debug adapter exited event should be deliverable after terminated");
241+
242+
// Bug 266 history: LSP4E used to react to a 'terminated' event from the
243+
// adapter by calling DSPProcess.terminate(), which in turn called
244+
// DSPDebugTarget.terminate() and sent a terminate or disconnect request back
245+
// to the adapter. This was contrary to the DAP guidelines where a 'terminated'
246+
// event indicates that the debuggee has ended and the adapter should be
247+
// allowed to shut down cleanly (emitting 'exited'). This test ensures no such
248+
// terminate or disconnect request is sent in response to the adapter's event.
249+
assertEquals(0, server.terminateRequestCount.get(),
250+
"LSP4E must not send terminate request back to adapter after receiving terminated event");
251+
assertEquals(0, server.disconnectRequestCount.get(),
252+
"LSP4E must not send disconnect request back to adapter after receiving terminated event");
253+
}
254+
255+
@Test
256+
void testTerminatedWithoutExitedCleansUpWithoutAdapterTerminate() throws Exception {
257+
ILaunch launch = newLaunch(ILaunchManager.RUN_MODE);
258+
259+
var params = new HashMap<String, Object>();
260+
params.put("type", "mock");
261+
params.put("request", "launch");
262+
params.put("program", "dummy");
263+
264+
var server = new MockTerminatedOnlyServer();
265+
var target = new TestDebugTarget(launch, params, server);
266+
267+
target.initialize(new NullProgressMonitor());
268+
269+
// Ensure the adapter's 'terminated' event has been sent
270+
TestUtils.waitForAndAssertCondition(5_000, server.terminatedDelivered::get);
271+
272+
// The fallback in DSPDebugTarget.terminated(TerminatedEventArguments) should
273+
// eventually clean up the session even if no 'exited' event is delivered.
274+
TestUtils.waitForAndAssertCondition(5_000, target::isTerminated);
275+
276+
assertTrue(target.isTerminated(), "Debug target should be terminated after adapter-only terminated event");
277+
278+
// As with the exited case, LSP4E must not send terminate or disconnect
279+
// requests back to the adapter as a reaction to its 'terminated' event.
280+
assertEquals(0, server.terminateRequestCount.get(),
281+
"LSP4E must not send terminate request back to adapter after receiving terminated event");
282+
assertEquals(0, server.disconnectRequestCount.get(),
283+
"LSP4E must not send disconnect request back to adapter after receiving terminated event");
284+
}
285+
}

0 commit comments

Comments
 (0)