From a95fe176935535502e70cf48ef2992b537487ad0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Novotn=C3=BD?= Date: Sat, 31 Oct 2026 11:09:00 +0100 Subject: [PATCH] fix: race tests for suspension ownership, and the three races they found (#47) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #47 § 4 asked for deterministic barriers and native cases. Each barrier runs the competing action inside the mocked JDI call that marks its gap (SuspensionRaceBarrierTest); two of them failed on the old code: - A frame tool and the dispatcher's lease: after the tool reserved the thread and found no evaluation, the dispatcher could lease it and run invoked code under the tool. lease() now waits while a frame tool reserves the thread (at most 2 s, then it is refused). - The hold check and a frame tool's retry: a set the dispatcher still processed vouched for the retry, but the dispatcher could release it before the retry read, and jdwp_set_local wrote into the frame of the thread's next stop. A retry now needs a hold only a tool ends — a published stop or a manual hold (SuspensionSession.userHoldsOn). The live case for a cancelled thread stop found the third: JDWP shows a thread that invokes from inside its event's handling as RUNNING even while it waits for a monitor, so the watchdog never looked at it. A thread shown running is now asked for its contended monitor too. Passing on the old code: a job still compiling refuses frame tools; the owner query then a new stop of the owner; a job's compilation then a replaced stop. Live: a stop-all released while its evaluation outlives the tool's wait. The live tests enable a thread-filtered request by suspending that thread only — a VM-wide resume broke evaluations in flight and made Order 1 flaky. --- docs/event-pipeline.md | 7 +- docs/testing.md | 7 +- docs/threading-and-safety.md | 4 +- .../java/one/edee/mcp/jdwp/JDWPTools.java | 16 +- .../one/edee/mcp/jdwp/SuspensionSession.java | 60 ++- .../one/edee/mcp/jdwp/SuspensionWatchdog.java | 9 +- .../JDWPToolsFrameToolsFreshFramesTest.java | 8 +- .../mcp/jdwp/SuspensionRaceBarrierTest.java | 465 ++++++++++++++++++ .../edee/mcp/jdwp/SuspensionSessionTest.java | 20 + .../edee/mcp/jdwp/SuspensionWatchdogTest.java | 36 +- .../mcp/jdwp/fastlogpoint/LiveSession.java | 16 + .../SuspensionOwnershipLiveTest.java | 101 +++- 12 files changed, 719 insertions(+), 30 deletions(-) create mode 100644 jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/SuspensionRaceBarrierTest.java diff --git a/docs/event-pipeline.md b/docs/event-pipeline.md index c57b4e0..0b5d2c0 100644 --- a/docs/event-pipeline.md +++ b/docs/event-pipeline.md @@ -391,15 +391,16 @@ Every invocation the server makes in the target runs inside an `EvaluationGuard` - **Count check.** T's suspend count must carry every hold the session knows: the anchor, every other covering context, and a manual hold. A count below that means a hold was undone outside the session — refused, nothing is resumed. A count above it is a set not routed yet, which routing exempts. - **Exemptions.** T is resumed once out of every other context covering it and joins that context's `exempt`; a manual hold of T is resumed too. The borrowed count is then T's only one, and the invocation can run. A stop-all routed while leases are active exempts every leased thread other than its own event thread (`route`). - **End (`unlease`).** T is suspended once back into each exempting context that still holds — one released meanwhile has compensated T already. A manual hold still present is put back. Last, the debt is resumed. +- **Frame tools first.** While a frame tool reads T (`reserve`), a lease of T waits until it is done — at most 2 s, then it is refused — or the invocation would run T under the tool ([threading-and-safety.md](threading-and-safety.md) § 5). - **Transfer.** If the anchor is released while its invocation runs (`jdwp_resume`, `jdwp_reset`, `nextStop`'s release), the borrower keeps its share (see Releases). `jdwp_resume_thread` and a step on a leased thread are refused ("an evaluation is running on T"). -**A lease spans invocations only, never frame reads.** JDI invalidates every `StackFrame` of a thread whenever the debugger resumes it, however many suspensions remain, and both an invocation and an exemption are such a resume. So the evaluator reads its frame first (the locals, `this`, the frame's class loader) and leases the thread only around its invocations; the fast-logpoint arming takes one lease per invocation group, not one across the whole arming. Frame tools that meet invalidated frames re-read them ([threading-and-safety.md](threading-and-safety.md) § 5). +**A lease spans invocations only, never frame reads.** JDI invalidates every `StackFrame` of a thread whenever the debugger resumes it, however many suspensions remain, and both an invocation and an exemption are such a resume. So the evaluator reads its frame first (the locals, `this`, the frame's class loader) and leases the thread only around its invocations; the fast-logpoint arming takes one lease per invocation group, not one across the whole arming. Frame tools that meet invalidated frames re-read them while a published stop or a manual hold keeps the thread at its stop ([threading-and-safety.md](threading-and-safety.md) § 5). ### The watchdog A dispatcher invocation — a condition, a logpoint, a fast-logpoint arming — can block on a monitor owned by a thread that a set queued behind it keeps parked. The dispatcher never reaches that set, and no tool releases a set before it is processed. (A tool's own invocation has a bounded wait instead: `InvocationRunner`, [threading-and-safety.md](threading-and-safety.md) § 5.) -`SuspensionWatchdog` runs per session on the daemon thread `jdi-suspension-watchdog`; the listener starts it with the session, and it stops once the session is closed. Every 500 ms it samples the session's dispatcher leases (`dispatcherInvocations()`). The lease object is the sample's token, so samples never carry over to a later invocation on the same thread. An invocation watched for more than 3 s whose thread shows status MONITOR in two consecutive samples has its monitor's owner looked up, and the decision made, during one balanced suspension of the invoking thread: suspend → `currentContendedMonitor().owningThread()` → `unblock(token, owner, monitor)` → resume. Suspended, the invoking thread cannot take the monitor and move on before the decision. `unblock` re-checks everything under the session monitor and releases nothing unless: +`SuspensionWatchdog` runs per session on the daemon thread `jdi-suspension-watchdog`; the listener starts it with the session, and it stops once the session is closed. Every 500 ms it samples the session's dispatcher leases (`dispatcherInvocations()`). The lease object is the sample's token, so samples never carry over to a later invocation on the same thread. An invocation is aged from the sample that first sees its thread running; a sample that finds the thread suspended — host-side work between two invocations of the lease — starts the next invocation afresh (#47 § 2). A thread JDWP shows as blocked (MONITOR) or running is asked for the monitor it waits for, during a balanced suspension; running is asked too because JDWP shows a thread that invokes from inside its event's handling as running even while it waits for a monitor (#47 § 4). An invocation older than 3 s that waits for the same monitor in two consecutive samples has its monitor's owner looked up, and the decision made, during one balanced suspension of the invoking thread: suspend → `currentContendedMonitor().owningThread()` → `unblock(token, owner, monitor)` → resume. Suspended, the invoking thread cannot take the monitor and move on before the decision. `unblock` re-checks everything under the session monitor and releases nothing unless: - the token is still a live dispatcher lease; - the owner is not leased, not reserved by a frame tool, and not manually held; @@ -416,7 +417,7 @@ A set with a fast breakpoint's stop event is never cancelled: its request carrie Records: `EVALUATION_UNBLOCKED` (the owner, the evaluating thread, the sets) and `EVENT_CANCELLED`. -Limits: a spin or a native wait (status RUNNING) is not detected, nor is a dependency on a thread a published stop holds. `jdwp_resume` or `jdwp_reconnect` recover those. +Limits: a spin or a native wait (no contended monitor) is not detected, nor is a dependency on a thread a published stop holds. `jdwp_resume` or `jdwp_reconnect` recover those. ### Attach diff --git a/docs/testing.md b/docs/testing.md index c91eb99..68b2b50 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -21,7 +21,7 @@ Live-JVM verification is done **manually** via the sandbox module, not automatic `fastlogpoint/FastBreakpointLiveTest` does the same for fast breakpoints: stops in both modes and policies, condition errors, inspection and steps at the user's frame, evaluations that overlap a stop-all, a clear while a thread is stopped in the method (the restore waits for its release, also for two entries in one class), and watchers by fast-breakpoint ID. Same opt-in: `mvn -pl jdwp-mcp-server test -Djdwp.live=true -Dtest=FastBreakpointLiveTest`. -`fastlogpoint/SuspensionOwnershipLiveTest` drives suspension ownership (issue #45) against the same forked target (`LiveSession`, fixture `FastLogpointTarget`): its `lock-holder` thread holds `FastLogpointTarget.LOCK`, so an evaluation can be made to wait on a monitor whose owner a stop-all parks. It checks that the watchdog frees a dispatcher evaluation stuck behind a queued stop-all, and that `jdwp_resume` releases only the stop it holds. Same opt-in: `mvn -pl jdwp-mcp-server test -Djdwp.live=true -Dtest=SuspensionOwnershipLiveTest`. +`fastlogpoint/SuspensionOwnershipLiveTest` drives suspension ownership (issue #45) against the same forked target (`LiveSession`, fixture `FastLogpointTarget`): its `lock-holder` thread holds `FastLogpointTarget.LOCK`, so an evaluation can be made to wait on a monitor whose owner a stop-all parks. It checks that the watchdog frees a dispatcher evaluation stuck behind a queued stop-all, and that `jdwp_resume` releases only the stop it holds; and, for #47 § 4, `jdwp_reset` during an evaluation, a fast logpoint and a fast breakpoint armed by one hit, a stop-all released while its evaluation outlives the tool's wait, a queued thread stop the watchdog cancels followed by a new stop of that thread, and a disconnect during an evaluation that does not return — each ending with every count at 0 and the workers making progress. Same opt-in: `mvn -pl jdwp-mcp-server test -Djdwp.live=true -Dtest=SuspensionOwnershipLiveTest`. ## 2. Mocking JDI @@ -68,13 +68,14 @@ Built on it: - `SuspensionSessionTest` — publication, presentation, releases with compensation, steps, `jdwp_reset`, leases and admission, ownership transfer; every scenario ends on the suspend counts. - `SuspensionSessionUnblockTest` — what `SuspensionSession.unblock` may release for the watchdog, how a cancelled set is settled, and what it never touches. -- `SuspensionWatchdogTest` — sampling driven by hand (`tick()`, an injected clock): the minimum age, two consecutive monitor samples, samples that belong to one invocation, a closed session stopping the watchdog. +- `SuspensionWatchdogTest` — sampling driven by hand (`tick()`, an injected clock): the minimum age, two consecutive samples on the same monitor, samples that belong to one invocation, a thread JDWP shows as running while it waits for a monitor, a closed session stopping the watchdog. +- `SuspensionRaceBarrierTest` — the races of #47 § 4, each held at its gap: the competing action runs inside the mocked JDI call that marks it, on the thread the dispatcher, the watchdog or a job would run it on. A job still compiling and a frame tool; a frame tool and the dispatcher's lease (which waits); the hold check and a frame tool's retry (only a published stop or a manual hold vouches for it); the watchdog's owner query and its decision; a job's compilation and a replaced stop. Mockito only, around the same feature: - `EvaluationGuardTest` — the depth marker, plus leases: a nested lease, a competing chain refused, a reconnect (`bind`) dropping the earlier connection's holds. - `InvocationRunnerTest` — the bounded wait: the result or the rethrown failure, `[STILL_RUNNING]` then exactly one late record, a busy thread refused, no late record in a later connection, a job given up before it started never run, a monitor's owner named with a balanced suspension. -- `JDWPToolsFrameToolsFreshFramesTest` — the frame tools re-read invalidated frames only while a hold the session knows lasted, and refuse a thread an evaluation runs on. +- `JDWPToolsFrameToolsFreshFramesTest` — the frame tools re-read invalidated frames only while a hold only a tool ends lasted, and refuse a thread an evaluation runs on. ### `TestReflectionUtils` diff --git a/docs/threading-and-safety.md b/docs/threading-and-safety.md index 63e7b40..53ca503 100644 --- a/docs/threading-and-safety.md +++ b/docs/threading-and-safety.md @@ -197,8 +197,8 @@ Invoked code can block — on a lock a parked thread holds, on a `join`. MCP too `jdwp_get_stack`, `jdwp_get_locals`, `jdwp_set_local` and `jdwp_get_breakpoint_context` run their work through `JDWPTools.withFreshFrames(thread, …)`: - **Refusal.** A thread an evaluation runs on is refused ("an evaluation is running on thread T — its frames are, or are about to be, the invoked code's"), before the suspended check: during an invocation the thread runs, and its frames belong to the invoked code. A job that outlived its tool's wait counts for its whole life (`InvocationRunner.isRunning`), also while it still compiles and holds no lease. -- **Reservation.** For the whole call the thread is reserved in the session (`reserve` / `unreserve`); the watchdog never releases a reserved thread. -- **Re-read.** A resume elsewhere — another stop-all's release, the fast-logpoint arming's VM-wide suspend — invalidates the frames the tool read, though the thread stays suspended. On `InvalidStackFrameException` the tool reads fresh frames and rebuilds its whole reply, at most three times (`FRAME_ATTEMPTS`) — but only while a hold the session knows lasted across the attempt. `SuspensionSession.holdsOn(T)` (the ids of the contexts holding T, a lease's exemption aside, plus the manual hold) is taken before the attempt and after its failure, and the two must intersect. Context ids are never reused, and a manual hold cannot be dropped and re-taken during a serialized tool call, so a common hold proves T never ran past its stop and the fresh frames are the same activations. Otherwise T may have run on and stopped elsewhere; the tool answers that it was resumed meanwhile, and `jdwp_set_local` adds that nothing was written. +- **Reservation.** For the whole call the thread is reserved in the session (`reserve` / `unreserve`); the watchdog never releases a reserved thread, and a lease of it waits until the call ends (`awaitUnreserved`, at most `RESERVATION_WAIT_MS` = 2 s, then the lease is refused). Only the dispatcher can lease the thread meanwhile — for a set of the thread's own that it processes, its condition say; tools run one at a time, and a job that outlived its tool's wait refuses the frame tool already. Without the wait the invocation would run the thread under the tool, and the frames it read next would be the invoked code's (#47 § 4). +- **Re-read.** A resume elsewhere — another stop-all's release, the fast-logpoint arming's VM-wide suspend — invalidates the frames the tool read, though the thread stays suspended. On `InvalidStackFrameException` the tool reads fresh frames and rebuilds its whole reply, at most three times (`FRAME_ATTEMPTS`) — but only while a hold only a tool ends lasted across the attempt. `SuspensionSession.userHoldsOn(T)` (the ids of the published stops holding T, plus the manual hold) is taken before the attempt and after its failure, and the two must intersect. Context ids are never reused, the dispatcher never releases a published stop, the watchdog never touches one or a manual hold, and tools run one at a time, so a common hold proves T never ran past its stop — and still holds it when the retry reads — and the fresh frames are the same activations. A set still queued or processed does not count: the dispatcher may release it between the check and the retry's read, and T run on to another stop, whose frames no invalidation would expose (#47 § 4). Otherwise T may have run on and stopped elsewhere; the tool answers that it was resumed meanwhile, and `jdwp_set_local` adds that nothing was written. - **Writes.** `jdwp_set_local` is retried only on that exception, which JDI raises before sending the write or when the target rejects the frame — either way the write did not land. Without a session (no connection yet), nothing is reserved or retried. diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JDWPTools.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JDWPTools.java index 3184139..06fb24b 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JDWPTools.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JDWPTools.java @@ -1255,11 +1255,13 @@ private ReferenceType forceLoad(String className) throws Exception { * stop's release, the arming's VM-wide suspend: JDI invalidates every frame of a resumed thread * though it stays suspended (issue #45 A12). Each attempt rebuilds its whole reply. *

- * It runs again only while a hold the session knows lasted across the attempt, so the fresh - * frames are the same activations; otherwise the thread may have run and stopped elsewhere, and - * the tool answers with {@code ifRunOn}. A write is retried only on this exception, which JDI - * raises before sending it or when the target rejects the frame — either way it did not land. - * Meanwhile the thread is reserved: the watchdog does not release it (A2). + * It runs again only while a hold only a tool ends — a published stop, a manual hold — lasted + * across the attempt: nothing releases it before the next read, so the fresh frames are the same + * activations. A set the dispatcher still processes may be released between the check and that + * read (#47 § 4). Otherwise the thread may have run and stopped elsewhere, and the tool answers + * with {@code ifRunOn}. A write is retried only on this exception, which JDI raises before sending + * it or when the target rejects the frame — either way it did not land. Meanwhile the thread is + * reserved: the watchdog does not release it (A2), and a lease of it waits until the tool is done. */ private String withFreshFrames(ThreadReference thread, String ifRunOn, Callable work) throws Exception { final SuspensionSession session = breakpointTracker.suspensionSession(); @@ -1269,7 +1271,7 @@ private String withFreshFrames(ThreadReference thread, String ifRunOn, Callable< } session.reserve(thread); try { - Set holds = session.holdsOn(thread); + Set holds = session.userHoldsOn(thread); for (int attempt = 1; ; attempt++) { final String evaluating = evaluationRunningOn(thread); if (evaluating != null) { @@ -1281,7 +1283,7 @@ private String withFreshFrames(ThreadReference thread, String ifRunOn, Callable< if (attempt == FRAME_ATTEMPTS) { throw invalidated; } - final Set now = session.holdsOn(thread); + final Set now = session.userHoldsOn(thread); if (Collections.disjoint(holds, now)) { return String.format("Error: thread %s was resumed while its frames were being read, and may " + "have run on since — %s", thread.name(), ifRunOn); diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/SuspensionSession.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/SuspensionSession.java index c54f218..25b55aa 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/SuspensionSession.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/SuspensionSession.java @@ -63,6 +63,10 @@ public final class SuspensionSession { * not routed yet. */ public static final long UNKNOWN_HOLD = -2L; + /** + * How long a {@link #lease} waits for a frame tool's {@link #reserve} of its thread to end. + */ + static final long RESERVATION_WAIT_MS = 2_000; /** * Request property marking a request whose events need their thread still stopped when the * dispatcher handles them even to skip them (a fast breakpoint's stop reads its arguments from @@ -735,15 +739,31 @@ public synchronized boolean stepPending(ThreadReference thread) { * {@link #MANUAL_HOLD} for a manual hold. A lease's exemption still counts — its invocation * returns to the same frames. While one hold lasts, the thread cannot run past its stop, so a * hold present in two snapshots proves its frames belong to the same activations in between - * (a frame tool's retry, issue #45 A12). Context ids are never reused. + * (an {@link InvocationRunner} job's admission). Context ids are never reused. */ public synchronized Set holdsOn(ThreadReference thread) { + return holdsOn(thread, false); + } + + /** + * {@link #holdsOn}, limited to the holds only a tool ends: the published stops holding + * {@code thread} and a manual hold. Nothing else releases those — the dispatcher releases only + * the sets it processes, the watchdog never touches a published stop or a manual hold, and tools + * run one at a time — so one of them found by a tool still holds the thread when the tool next + * reads its frames. A set still queued or processed does not vouch for that: the dispatcher may + * release it at any moment, and the thread run on (a frame tool's retry, #47 § 4). + */ + public synchronized Set userHoldsOn(ThreadReference thread) { + return holdsOn(thread, true); + } + + private Set holdsOn(ThreadReference thread, boolean publishedOnly) { if (closed) { return Set.of(); } final Set holds = new LinkedHashSet<>(); for (Context context : contexts) { - if (claims(context, thread)) { + if ((!publishedOnly || context.state == State.HELD) && claims(context, thread)) { holds.add(context.id); } } @@ -876,11 +896,15 @@ public synchronized StartState acknowledgeStart(long timeoutMs) throws Interrupt * The dispatcher may invoke on the event thread of the set it processes. Any other caller needs * the thread stopped at an event of its own that a tool has shown, and no set covering it still * queued, processed or unseen. A tool's job must also still find one of the holds it was started - * under ({@link #admittedUnder}). + * under ({@link #admittedUnder}). While a frame tool reads the thread ({@link #reserve}), the + * lease waits for it: the invocation would run the thread under the tool, and the frames the tool + * reads next would be the invoked code's (#47 § 4). * - * @throws EvaluationRefusedException when the invocation is not admitted + * @throws EvaluationRefusedException when the invocation is not admitted, or a frame tool still + * reads the thread after {@link #RESERVATION_WAIT_MS} */ public synchronized void lease(ThreadReference thread) { + awaitUnreserved(thread); if (closed) { return; } @@ -968,8 +992,9 @@ public synchronized void unlease(ThreadReference thread) { // ── Watchdog (A1) and reservations (A2) ── /** - * A frame tool starts working on {@code thread}: the watchdog leaves it alone until - * {@link #unreserve}. + * A frame tool starts working on {@code thread}: the watchdog leaves it alone, and a + * {@link #lease} of it waits, until {@link #unreserve}. A frame tool never invokes on the thread + * it reserves. */ public synchronized void reserve(ThreadReference thread) { reserved.merge(thread, 1, Integer::sum); @@ -980,6 +1005,29 @@ public synchronized void reserve(ThreadReference thread) { */ public synchronized void unreserve(ThreadReference thread) { reserved.computeIfPresent(thread, (key, tools) -> tools == 1 ? null : tools - 1); + notifyAll(); + } + + /** + * Waits, releasing the monitor, until no frame tool reserves {@code thread}. A tool's reads are a + * few JDWP commands; one still reading after {@link #RESERVATION_WAIT_MS} refuses the lease. + */ + private void awaitUnreserved(ThreadReference thread) { + final long deadline = System.nanoTime() + RESERVATION_WAIT_MS * 1_000_000L; + while (!closed && reserved.containsKey(thread)) { + final long remainingMs = (deadline - System.nanoTime()) / 1_000_000L; + if (remainingMs <= 0) { + throw new EvaluationRefusedException("a tool is reading the frames of thread " + safeName(thread) + + " — evaluate again when it has finished"); + } + try { + wait(remainingMs); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new EvaluationRefusedException("interrupted while a tool read the frames of thread " + + safeName(thread)); + } + } } /** diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/SuspensionWatchdog.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/SuspensionWatchdog.java index 9843309..b83e35f 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/SuspensionWatchdog.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/SuspensionWatchdog.java @@ -29,7 +29,9 @@ * afresh (#47 § 2). One running for more than {@link #MIN_AGE_NANOS} whose thread is blocked on the * same monitor in two consecutive samples has the monitor's owner asked for — a balanced suspend of * the invoking thread around each query — and {@link SuspensionSession#unblock} releases that owner - * if, under the session's monitor, it is held by such sets alone. Worker invocations are not watched: their tool's wait is bounded + * if, under the session's monitor, it is held by such sets alone. A thread shown running is asked as + * well: JDWP shows one that invokes inside its event's handling as running even while it waits for a + * monitor (#47 § 4). Worker invocations are not watched: their tool's wait is bounded * ({@link InvocationRunner}). Spins, native waits and dependencies on a published stop are not * detected: {@code jdwp_resume} or {@code jdwp_reconnect} recover those. */ @@ -136,7 +138,10 @@ private void sample(SuspensionSession.Invocation invocation, long now) { if (now - since < MIN_AGE_NANOS) { return; } - if (statusOf(invocation.thread()) != ThreadReference.THREAD_STATUS_MONITOR) { + // JDWP shows a thread that invokes from inside its event's handling as RUNNING even while it + // waits for a monitor (#47 § 4), so a running one is asked too; it names no monitor if it runs. + final int status = statusOf(invocation.thread()); + if (status != ThreadReference.THREAD_STATUS_MONITOR && status != ThreadReference.THREAD_STATUS_RUNNING) { forgetSamples(token); return; } diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsFrameToolsFreshFramesTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsFrameToolsFreshFramesTest.java index 4b48f3f..dc64c6e 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsFrameToolsFreshFramesTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/JDWPToolsFrameToolsFreshFramesTest.java @@ -79,7 +79,7 @@ jdiService, breakpointTracker, mock(WatcherManager.class), mock(JdiExpressionEva session = mock(SuspensionSession.class); when(breakpointTracker.suspensionSession()).thenReturn(session); // The stop holding the thread lasts unless a test says otherwise. - when(session.holdsOn(thread)).thenReturn(Set.of(STOP)); + when(session.userHoldsOn(thread)).thenReturn(Set.of(STOP)); } private static StackFrame staleFrame() { @@ -137,7 +137,7 @@ void localsAreReadAfresh() throws Exception { when(thread.frame(0)).thenReturn(stale, fresh); // Another thread's stop-all (9) was released — the resume that invalidated the frames — while // the thread's own stop lasted. - when(session.holdsOn(thread)).thenReturn(Set.of(STOP, 9L), Set.of(STOP)); + when(session.userHoldsOn(thread)).thenReturn(Set.of(STOP, 9L), Set.of(STOP)); final String result = tools.jdwp_get_locals(THREAD_ID, 0); @@ -203,7 +203,7 @@ class MayHaveRunOn { @DisplayName("jdwp_set_local writes nothing into a frame that may be another activation, and says so") void setLocalWritesNothing() throws Exception { // The stop that held the thread was released; it ran on and stopped again, under stop 7. - when(session.holdsOn(thread)).thenReturn(Set.of(STOP), Set.of(7L)); + when(session.userHoldsOn(thread)).thenReturn(Set.of(STOP), Set.of(7L)); final LocalVariable count = mock(LocalVariable.class); final Type intType = mock(Type.class); final IntegerValue fortyTwo = mock(IntegerValue.class); @@ -228,7 +228,7 @@ void setLocalWritesNothing() throws Exception { @Test @DisplayName("a read tool renders no frames that may be another activation") void readToolAsksToCallAgain() throws Exception { - when(session.holdsOn(thread)).thenReturn(Set.of(STOP), Set.of()); + when(session.userHoldsOn(thread)).thenReturn(Set.of(STOP), Set.of()); final StackFrame stale = staleFrame(); final StackFrame elsewhere = frameAt("com.example.Elsewhere", "run", 7); when(thread.frames()).thenReturn(List.of(stale), List.of(elsewhere)); diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/SuspensionRaceBarrierTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/SuspensionRaceBarrierTest.java new file mode 100644 index 0000000..a4d6939 --- /dev/null +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/SuspensionRaceBarrierTest.java @@ -0,0 +1,465 @@ +package one.edee.mcp.jdwp; + +import com.sun.jdi.IntegerValue; +import com.sun.jdi.InvalidStackFrameException; +import com.sun.jdi.LocalVariable; +import com.sun.jdi.ObjectReference; +import com.sun.jdi.StackFrame; +import com.sun.jdi.ThreadReference; +import com.sun.jdi.Type; +import com.sun.jdi.Value; +import com.sun.jdi.request.EventRequest; +import one.edee.mcp.jdwp.discovery.JvmDiscoveryService; +import one.edee.mcp.jdwp.evaluation.JdiExpressionEvaluator; +import one.edee.mcp.jdwp.watchers.WatcherManager; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; + +import java.util.List; +import java.util.concurrent.Callable; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicLong; +import java.util.concurrent.atomic.AtomicReference; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** + * The races of issue #47 § 4, each held at its gap: the competing action runs inside the mocked JDI + * call that marks the gap — on the thread the dispatcher, the watchdog or a tool's job would run it + * on — so every run takes the same interleaving. Every scenario ends on the suspend counts the + * target would really have ({@link FakeSuspendModel}). + */ +@DisplayName("SuspensionSession — races held at their barriers (#47 § 4)") +class SuspensionRaceBarrierTest { + + private static final long WAIT_SECONDS = 5; + + private FakeSuspendModel model; + private FakeSuspendModel.FakeThread t; + private FakeSuspendModel.FakeThread u; + private EventHistory history; + private SuspensionSession session; + private EvaluationGuard guard; + private JdiExpressionEvaluator evaluator; + private JDWPTools tools; + /** + * Plays the event dispatcher: the sets it processes and the leases it takes are its own. + */ + private ExecutorService dispatcher; + + @BeforeEach + void setUp() throws Exception { + model = new FakeSuspendModel(); + t = model.thread("t"); + u = model.thread("u"); + history = new EventHistory(); + final BreakpointTracker tracker = new BreakpointTracker(); + session = tracker.openSuspensionSession(history); + guard = new EvaluationGuard(); + guard.bind(session); + evaluator = mock(JdiExpressionEvaluator.class); + final JDIConnectionService jdiService = mock(JDIConnectionService.class); + when(jdiService.getVM()).thenReturn(model.vm); + tools = JDWPToolsTestSupport.newTools(jdiService, tracker, mock(WatcherManager.class), evaluator, history, guard, + new JvmDiscoveryService()); + dispatcher = Executors.newSingleThreadExecutor(r -> new Thread(r, "test-dispatcher")); + } + + @AfterEach + void tearDown() throws Exception { + dispatcher.submit(SuspensionSession::endProcessing).get(WAIT_SECONDS, TimeUnit.SECONDS); + dispatcher.shutdownNow(); + } + + @Nested + @DisplayName("reservation, then the evaluation check, then lease entry") + class ReservationAndLeaseEntry { + + @Test + @DisplayName("a tool's job still compiling holds no lease yet, but a frame tool refuses its thread before reading anything") + void compilingJobRefusesFrameTools() throws Exception { + presentedStop(t, EventRequest.SUSPEND_EVENT_THREAD, 1); + final CountDownLatch compiling = new CountDownLatch(1); + final CountDownLatch compiled = new CountDownLatch(1); + doAnswer(invocation -> { + compiling.countDown(); + assertThat(compiled.await(WAIT_SECONDS, TimeUnit.SECONDS)).isTrue(); + return null; + }).when(evaluator).configureCompilerClasspath(t.ref); + final StackFrame frame = mock(StackFrame.class); + when(t.ref.frame(0)).thenReturn(frame); + final Value result = mock(Value.class); + when(evaluator.evaluate(eq(frame), eq("slow()"), any())).thenAnswer(invocation -> invoked(t, result)); + final CompletableFuture evaluation = + CompletableFuture.supplyAsync(() -> tools.jdwp_evaluate_expression(id(t), "slow()", null)); + assertThat(compiling.await(WAIT_SECONDS, TimeUnit.SECONDS)).isTrue(); + assertThat(guard.isEvaluating(t.ref)).as("no lease yet").isFalse(); + + final List replies = List.of( + tools.jdwp_get_locals(id(t), 0), + tools.jdwp_set_local(id(t), 0, "count", "42"), + tools.jdwp_get_stack(id(t), null, null)); + + assertThat(replies).allSatisfy(reply -> assertThat(reply).startsWith("Error: an evaluation is running on thread t")); + verify(t.ref, never()).frame(anyInt()); + verify(t.ref, never()).frames(); + + compiled.countDown(); + assertThat(evaluation.get(WAIT_SECONDS, TimeUnit.SECONDS)).startsWith("Result of slow()"); + assertThat(t.count).as("back at its stop after the invocation").isEqualTo(1); + session.releaseAll(); + assertAllRunning(); + } + + @Test + @DisplayName("a frame tool has reserved the thread and found no evaluation: the dispatcher's lease waits until the tool is done") + void dispatcherLeaseWaitsForTheFrameTool() throws Exception { + // t is at a breakpoint whose condition the dispatcher is about to evaluate on t. + final SuspensionSession.Context own = session.route( + model.breakpointEvent(t, EventRequest.SUSPEND_EVENT_THREAD), t.ref, false); + onDispatcher(() -> { + assertThat(session.begin(own)).isTrue(); + session.beginProcessing(own); + return null; + }); + final CountDownLatch invoking = new CountDownLatch(1); + final AtomicBoolean invokedDuringTheRead = new AtomicBoolean(); + final AtomicReference> condition = new AtomicReference<>(); + final StackFrame frame = mock(StackFrame.class); + when(t.ref.frame(0)).thenAnswer(invocation -> { + // The gap: the tool reserved t and found no evaluation; now the dispatcher leases t. + condition.set(dispatcher.submit(() -> { + try (EvaluationGuard.Lease lease = guard.lease(t.ref)) { + model.beginInvocation(t); + invoking.countDown(); + model.completeInvocation(t); + } + return null; + })); + invokedDuringTheRead.set(invoking.await(300, TimeUnit.MILLISECONDS)); + return frame; + }); + + final String reply = tools.jdwp_get_locals(id(t), 0); + + assertThat(reply).startsWith("Local variables in frame 0"); + assertThat(invokedDuringTheRead).as("t ran invoked code while its frames were read").isFalse(); + condition.get().get(WAIT_SECONDS, TimeUnit.SECONDS); + assertThat(invoking.getCount()).as("the condition ran once the tool was done").isZero(); + onDispatcher(() -> { + SuspensionSession.endProcessing(); + assertThat(session.finish(own)).as("the condition was false").isFalse(); + return null; + }); + assertAllRunning(); + } + + @Test + @DisplayName("a frame tool that does not finish: the lease is refused after the wait, and nothing was taken") + void leaseIsRefusedWhenTheToolDoesNotFinish() throws Exception { + final SuspensionSession.Context own = session.route( + model.breakpointEvent(t, EventRequest.SUSPEND_EVENT_THREAD), t.ref, false); + session.reserve(t.ref); + final long started = System.nanoTime(); + assertThatThrownBy(() -> onDispatcher(() -> { + assertThat(session.begin(own)).isTrue(); + session.beginProcessing(own); + return guard.lease(t.ref); + })).hasRootCauseInstanceOf(EvaluationRefusedException.class) + .hasMessageContaining("a tool is reading the frames of thread t"); + + assertThat(TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - started)).as("it waited for the tool first") + .isGreaterThan(SuspensionSession.RESERVATION_WAIT_MS / 2); + assertThat(guard.isEvaluating(t.ref)).isFalse(); + assertThat(session.dispatcherInvocations()).isEmpty(); + session.unreserve(t.ref); + onDispatcher(() -> { + SuspensionSession.endProcessing(); + assertThat(session.finish(own)).isFalse(); + return null; + }); + assertAllRunning(); + } + } + + @Nested + @DisplayName("the hold check, then a frame tool's retry") + class HoldCheckAndRetry { + + @Test + @DisplayName("a set the dispatcher may release does not vouch for the retry: nothing is written to the frames of the thread's next stop") + void unpublishedSetDoesNotVouchForTheRetry() throws Exception { + // u's stop-all is being processed (its condition evaluated, say): t is parked by it. + final SuspensionSession.Context stopAll = session.route( + model.breakpointEvent(u, EventRequest.SUSPEND_ALL), u.ref, false); + onDispatcher(() -> { + assertThat(session.begin(stopAll)).isTrue(); + session.beginProcessing(stopAll); + return null; + }); + final IntegerValue fortyTwo = mock(IntegerValue.class); + when(model.vm.mirrorOf(42)).thenReturn(fortyTwo); + final LocalVariable count = intLocal("count"); + final StackFrame stale = frameWith("count", count); + doThrow(new InvalidStackFrameException("Thread has been resumed")).when(stale).setValue(count, fortyTwo); + final StackFrame next = frameWith("count", count); + final AtomicReference nextStop = new AtomicReference<>(); + // The gap: the stop-all held t at the check; before the retry reads, the dispatcher finishes + // it (its condition was false), and t runs on to a breakpoint R. + final Callable runOn = () -> { + if (nextStop.get() == null) { + onDispatcher(() -> { + SuspensionSession.endProcessing(); + assertThat(session.finish(stopAll)).isFalse(); + return null; + }); + assertThat(t.count).as("released with the stop-all").isZero(); + nextStop.set(session.route(model.breakpointEvent(t, EventRequest.SUSPEND_EVENT_THREAD), t.ref, false)); + } + return null; + }; + when(t.ref.frame(0)).thenReturn(stale).thenAnswer(invocation -> { + runOn.call(); + return next; + }); + + final String reply = tools.jdwp_set_local(id(t), 0, "count", "42"); + + assertThat(reply).startsWith("Error: thread t was resumed").contains("nothing was written"); + verify(next, never()).setValue(any(), any()); + runOn.call(); + assertThat(session.begin(nextStop.get())).isTrue(); + session.candidate(nextStop.get(), new BreakpointTracker.LastBreakpoint(t.ref, 2)); + assertThat(session.finish(nextStop.get())).isTrue(); + assertThat(session.nextStop(0).outcome()).isEqualTo(SuspensionSession.Outcome.STOPPED); + assertThat(t.count).isEqualTo(1); + session.releaseAll(); + assertAllRunning(); + } + + @Test + @DisplayName("a published stop vouches for the retry: the dispatcher releasing another set meanwhile cannot move the thread") + void publishedStopVouchesForTheRetry() throws Exception { + presentedStop(t, EventRequest.SUSPEND_EVENT_THREAD, 1); + final SuspensionSession.Context stopAll = session.route( + model.breakpointEvent(u, EventRequest.SUSPEND_ALL), u.ref, false); + onDispatcher(() -> { + assertThat(session.begin(stopAll)).isTrue(); + session.beginProcessing(stopAll); + return null; + }); + final IntegerValue fortyTwo = mock(IntegerValue.class); + when(model.vm.mirrorOf(42)).thenReturn(fortyTwo); + final LocalVariable count = intLocal("count"); + final StackFrame stale = frameWith("count", count); + doThrow(new InvalidStackFrameException("Thread has been resumed")).when(stale).setValue(count, fortyTwo); + final StackFrame fresh = frameWith("count", count); + when(t.ref.frame(0)).thenReturn(stale).thenAnswer(invocation -> { + onDispatcher(() -> { + SuspensionSession.endProcessing(); + assertThat(session.finish(stopAll)).isFalse(); + return null; + }); + return fresh; + }); + + final String reply = tools.jdwp_set_local(id(t), 0, "count", "42"); + + assertThat(reply).startsWith("Variable 'count' set to 42"); + verify(fresh).setValue(count, fortyTwo); + assertThat(t.count).as("still at its published stop").isEqualTo(1); + session.releaseAll(); + assertAllRunning(); + } + } + + @Nested + @DisplayName("the owner query, then the watchdog's decision") + class OwnerQueryAndUnblock { + + @Test + @DisplayName("the sampled owner leaves the monitor and stops at a new breakpoint before the decision: that stop is not cancelled") + void ownerThatStoppedElsewhereKeepsItsStop() throws Exception { + // The dispatcher evaluates a condition on t; u, running, owns the monitor the invoked code waits for. + final SuspensionSession.Context own = session.route( + model.breakpointEvent(t, EventRequest.SUSPEND_EVENT_THREAD), t.ref, false); + onDispatcher(() -> { + assertThat(session.begin(own)).isTrue(); + session.beginProcessing(own); + session.lease(t.ref); + model.beginInvocation(t); + return null; + }); + final AtomicReference queued = new AtomicReference<>(); + final ObjectReference monitor = mock(ObjectReference.class); + when(monitor.owningThread()).thenAnswer(invocation -> { + if (queued.get() != null) { + return null; + } + // The gap: asked, u owns the monitor; before the session decides, u leaves it and stops at Q. + queued.set(session.route(model.breakpointEvent(u, EventRequest.SUSPEND_EVENT_THREAD), u.ref, false)); + return u.ref; + }); + when(t.ref.currentContendedMonitor()).thenReturn(monitor); + t.status = ThreadReference.THREAD_STATUS_MONITOR; + final AtomicLong now = new AtomicLong(); + final SuspensionWatchdog watchdog = new SuspensionWatchdog(session, now::get); + watchdog.tick(); + now.addAndGet(SuspensionWatchdog.MIN_AGE_NANOS); + watchdog.tick(); + now.addAndGet(TimeUnit.MILLISECONDS.toNanos(SuspensionWatchdog.PERIOD_MS)); + watchdog.tick(); + + assertThat(queued.get()).as("the watchdog asked for the owner").isNotNull(); + assertThat(session.stateOf(queued.get())).isEqualTo(SuspensionSession.State.QUEUED); + assertThat(u.count).isEqualTo(1); + assertThat(types()).doesNotContain("EVENT_CANCELLED", "EVALUATION_UNBLOCKED"); + assertThat(model.invocationRuns(t)).as("the brief suspension around the query is undone").isTrue(); + + // The invocation ends, the dispatcher finishes its set, then Q, which publishes. + t.status = ThreadReference.THREAD_STATUS_RUNNING; + onDispatcher(() -> { + model.completeInvocation(t); + session.unlease(t.ref); + SuspensionSession.endProcessing(); + assertThat(session.finish(own)).isFalse(); + assertThat(session.begin(queued.get())).isTrue(); + session.candidate(queued.get(), new BreakpointTracker.LastBreakpoint(u.ref, 2)); + assertThat(session.finish(queued.get())).isTrue(); + return null; + }); + assertThat(session.nextStop(0).outcome()).isEqualTo(SuspensionSession.Outcome.STOPPED); + assertThat(u.count).as("Q holds u").isEqualTo(1); + session.releaseAll(); + assertAllRunning(); + } + } + + @Nested + @DisplayName("compilation, then a replaced stop") + class CompilationAndReplacedStop { + + @Test + @DisplayName("a job that outlived its wait while compiling, whose stop was released and replaced by a new one: its invocation is refused, the new stop untouched") + void jobOfAReleasedStopDoesNotInvokeAtTheNextOne() throws Exception { + presentedStop(t, EventRequest.SUSPEND_EVENT_THREAD, 1); + final ExecutorService pool = InvocationRunner.newPool(); + try { + final InvocationRunner runner = new InvocationRunner(pool, 100, history, () -> session); + final CountDownLatch compiled = new CountDownLatch(1); + assertThatThrownBy(() -> runner.run(t.ref, "evaluating slow()", () -> { + assertThat(compiled.await(WAIT_SECONDS, TimeUnit.SECONDS)).isTrue(); + return invoked(t, "late value"); + })).isInstanceOf(InvocationRunner.StillRunning.class); + + // The user releases the stop; t runs on and stops at the same place again, and that + // stop is presented — then the job finishes compiling. + assertThat(session.releaseAll().released()).hasSize(1); + assertThat(t.count).isZero(); + final SuspensionSession.Context replaced = presentedStop(t, EventRequest.SUSPEND_EVENT_THREAD, 1); + compiled.countDown(); + + final EventHistory.DebugEvent late = awaitEvent("EVALUATION_COMPLETED_LATE"); + assertThat(late.summary()).contains("failed: thread t was released from the stop this evaluation started at"); + assertThat(t.loans).as("nothing was invoked").isZero(); + assertThat(t.count).isEqualTo(1); + assertThat(session.stateOf(replaced)).isEqualTo(SuspensionSession.State.HELD); + assertThat(guard.isEvaluating(t.ref)).isFalse(); + session.releaseAll(); + assertAllRunning(); + } finally { + pool.shutdownNow(); + } + } + } + + // ── helpers ── + + /** + * A stop published on {@code thread} and presented, as {@code jdwp_resume_until_event} shows it. + */ + private SuspensionSession.Context presentedStop(FakeSuspendModel.FakeThread thread, int policy, int breakpointId) + throws Exception { + final SuspensionSession.Context context = session.route(model.breakpointEvent(thread, policy), thread.ref, false); + assertThat(session.begin(context)).isTrue(); + session.candidate(context, new BreakpointTracker.LastBreakpoint(thread.ref, breakpointId)); + assertThat(session.finish(context)).isTrue(); + assertThat(session.nextStop(0).outcome()).isEqualTo(SuspensionSession.Outcome.STOPPED); + return context; + } + + /** + * What the evaluator does once it has compiled: leases {@code thread} and invokes on it. + */ + private V invoked(FakeSuspendModel.FakeThread thread, V value) throws Exception { + try (EvaluationGuard.Lease lease = guard.lease(thread.ref)) { + model.beginInvocation(thread); + model.completeInvocation(thread); + } + return value; + } + + private V onDispatcher(Callable work) throws Exception { + return dispatcher.submit(work).get(WAIT_SECONDS, TimeUnit.SECONDS); + } + + private static long id(FakeSuspendModel.FakeThread thread) { + return thread.ref.uniqueID(); + } + + private static LocalVariable intLocal(String name) throws Exception { + final LocalVariable local = mock(LocalVariable.class); + final Type intType = mock(Type.class); + when(local.name()).thenReturn(name); + when(local.typeName()).thenReturn("int"); + when(local.type()).thenReturn(intType); + when(intType.name()).thenReturn("int"); + return local; + } + + private static StackFrame frameWith(String name, LocalVariable local) throws Exception { + final StackFrame frame = mock(StackFrame.class); + when(frame.visibleVariableByName(name)).thenReturn(local); + return frame; + } + + private EventHistory.DebugEvent awaitEvent(String type) throws Exception { + final long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(WAIT_SECONDS); + while (System.nanoTime() < deadline) { + for (EventHistory.DebugEvent event : history.getRecent(EventHistory.MAX_EVENTS)) { + if (event.type().equals(type)) { + return event; + } + } + Thread.sleep(10); + } + throw new AssertionError("no " + type + " in " + history.getRecent(EventHistory.MAX_EVENTS)); + } + + private List types() { + return history.getRecent(EventHistory.MAX_EVENTS).stream().map(EventHistory.DebugEvent::type).toList(); + } + + private void assertAllRunning() { + assertThat(model.counts()).allSatisfy((name, count) -> assertThat(count).as(name).isZero()); + } +} diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/SuspensionSessionTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/SuspensionSessionTest.java index d1b2c5e..300572f 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/SuspensionSessionTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/SuspensionSessionTest.java @@ -433,6 +433,26 @@ void holdsOnNamesWhatKeepsTheThreadAtItsStop() throws Exception { assertThat(session.holdsOn(u.ref)).isEmpty(); } + @Test + @DisplayName("the holds only a tool ends: published stops and a manual hold, not a set still queued or processed (#47 § 4)") + void userHoldsOnLeavesOutUnpublishedSets() throws Exception { + assertThat(session.suspendThread(w.ref)).isNull(); + final SuspensionSession.Context own = presented(stopOn(t, EventRequest.SUSPEND_EVENT_THREAD, 1)); + final SuspensionSession.Context queued = session.route(model.breakpointEvent(u, EventRequest.SUSPEND_ALL), u.ref, + false); + + assertThat(session.holdsOn(t.ref)).containsExactly(own.id(), queued.id()); + assertThat(session.userHoldsOn(t.ref)).containsExactly(own.id()); + assertThat(session.userHoldsOn(w.ref)).containsExactly(SuspensionSession.MANUAL_HOLD); + assertThat(session.userHoldsOn(u.ref)).as("only its own set, not processed yet").isEmpty(); + + session.begin(queued); + assertThat(session.userHoldsOn(t.ref)).as("processing does not publish it").containsExactly(own.id()); + assertThat(session.finish(queued)).isFalse(); + session.releaseAll(); + assertThat(model.counts()).allSatisfy((name, count) -> assertThat(count).isZero()); + } + @Test @DisplayName("the dispatcher may invoke on the thread of the set it processes, though nobody has seen it") void dispatcherLeaseIsAdmittedByConstruction() { diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/SuspensionWatchdogTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/SuspensionWatchdogTest.java index ac029b4..843f591 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/SuspensionWatchdogTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/SuspensionWatchdogTest.java @@ -106,8 +106,12 @@ void youngInvocationIsLeftAlone() throws Exception { } @Test - @DisplayName("a sample that is not blocked on a monitor starts the count again") + @DisplayName("a sample that finds the invocation waiting for no monitor starts the count again") void unblockedSampleResetsTheStreak() throws Exception { + final ObjectReference monitor = mock(ObjectReference.class); + when(monitor.owningThread()).thenReturn(u.ref); + // Blocked, then running on (no monitor), then blocked again. + when(t.ref.currentContendedMonitor()).thenReturn(monitor, null, monitor); watchdog.tick(); advance(SuspensionWatchdog.MIN_AGE_NANOS); t.status = ThreadReference.THREAD_STATUS_MONITOR; @@ -120,6 +124,36 @@ void unblockedSampleResetsTheStreak() throws Exception { assertThat(u.count).isEqualTo(1); } + @Test + @DisplayName("an invocation JDWP shows as running though it waits for a monitor — made from inside its event's handling — is found (#47 § 4)") + void monitorWaitShownAsRunningIsFound() throws Exception { + t.status = ThreadReference.THREAD_STATUS_RUNNING; + watchdog.tick(); + advance(SuspensionWatchdog.MIN_AGE_NANOS); + watchdog.tick(); + advance(TimeUnit.MILLISECONDS.toNanos(SuspensionWatchdog.PERIOD_MS)); + watchdog.tick(); + + assertThat(u.count).isZero(); + assertThat(model.invocationRuns(t)).as("the brief suspension around the query is undone").isTrue(); + } + + @Test + @DisplayName("a waiting or sleeping invocation is not asked for a monitor owner") + void waitingInvocationIsNotAsked() throws Exception { + for (int status : new int[]{ThreadReference.THREAD_STATUS_WAIT, ThreadReference.THREAD_STATUS_SLEEPING}) { + t.status = status; + watchdog.tick(); + advance(SuspensionWatchdog.MIN_AGE_NANOS); + watchdog.tick(); + advance(TimeUnit.MILLISECONDS.toNanos(SuspensionWatchdog.PERIOD_MS)); + watchdog.tick(); + } + + assertThat(u.count).isEqualTo(1); + verify(t.ref, never()).currentContendedMonitor(); + } + @Test @DisplayName("samples belong to one invocation: the next invocation on the thread starts young") void nextInvocationStartsAfresh() throws Exception { diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/fastlogpoint/LiveSession.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/fastlogpoint/LiveSession.java index 72e70fb..b26c3ae 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/fastlogpoint/LiveSession.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/fastlogpoint/LiveSession.java @@ -1,6 +1,7 @@ package one.edee.mcp.jdwp.fastlogpoint; import com.sun.jdi.Method; +import com.sun.jdi.ThreadReference; import com.sun.jdi.VirtualMachine; import com.sun.jdi.request.EventRequest; import one.edee.mcp.jdwp.*; @@ -223,4 +224,19 @@ static void enableSuspended(EventRequest request) { vm.resume(); } } + + /** + * {@link #enableSuspended(EventRequest)} for a request filtered to {@code thread}: only that + * thread can raise its event, so only it is suspended. A VM-wide resume while an evaluation runs + * on another thread would invalidate the frame the evaluator reads next ("Thread has been + * resumed"). + */ + static void enableSuspended(EventRequest request, ThreadReference thread) { + thread.suspend(); + try { + request.enable(); + } finally { + thread.resume(); + } + } } diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/fastlogpoint/SuspensionOwnershipLiveTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/fastlogpoint/SuspensionOwnershipLiveTest.java index 8f86097..9e1e710 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/fastlogpoint/SuspensionOwnershipLiveTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/fastlogpoint/SuspensionOwnershipLiveTest.java @@ -83,7 +83,7 @@ void watchdogFreesAnEvaluationStuckBehindAQueuedStop() throws Exception { parkHolder.addThreadFilter(holder); parkHolder.addCountFilter(1); parkHolder.setSuspendPolicy(EventRequest.SUSPEND_ALL); - enableSuspended(parkHolder); + enableSuspended(parkHolder, holder); await("the watchdog to release the holder", () -> eventTypes().contains("EVALUATION_UNBLOCKED")); assertThat(history.getRecent(EventHistory.MAX_EVENTS)).as("the holder's stop was cancelled, not shown") @@ -143,7 +143,7 @@ void resumeReleasesOnlyHeldStops() throws Exception { queuedStop.addThreadFilter(holder); queuedStop.addCountFilter(1); queuedStop.setSuspendPolicy(EventRequest.SUSPEND_ALL); - enableSuspended(queuedStop); + enableSuspended(queuedStop, holder); await("the holder's stop-all to queue", () -> sessionState().contains("QUEUED")); assertThat(held.suspendCount()).as("its own stop and the queued stop-all").isEqualTo(2); @@ -233,6 +233,103 @@ void coDeliveredArmingHits() throws Exception { @Test @Order(5) + @DisplayName("a stop-all whose evaluation outlives its wait is released: the other threads run at once, the evaluating thread once it ends (#47 § 4)") + void stopAllAnchorHandedToARunningEvaluation() throws Exception { + final VirtualMachine vm = connection.getVM(); + final ThreadReference evaluating = thread("worker-0"); + final ThreadReference other = thread("worker-1"); + final ThreadReference holder = thread("lock-holder"); + final Location probe = vm.classesByName(TARGET).get(0).locationsOfLine(lineOf("// MARK: probe")).get(0); + final BreakpointRequest stopAll = vm.eventRequestManager().createBreakpointRequest(probe); + stopAll.setSuspendPolicy(EventRequest.SUSPEND_ALL); + stopAll.addThreadFilter(evaluating); + stopAll.addCountFilter(1); + final int stopId = session.tracker.registerBreakpoint(stopAll); + enableSuspended(stopAll); + try { + assertThat(tools.jdwp_resume_until_event((int) TIMEOUT.toMillis())).contains("Thread: worker-0"); + assertThat(other.suspendCount()).as("parked by the stop-all").isEqualTo(1); + + final String reply = tools.jdwp_evaluate_expression(evaluating.uniqueID(), + "{ java.util.concurrent.locks.LockSupport.parkNanos(25_000_000_000L); return 42; }", 0); + assertThat(reply).startsWith("[STILL_RUNNING]").contains("thread worker-0"); + + // The user releases the stop-all while the evaluation still runs: worker-0's share of it now + // belongs to the evaluation; every other thread runs at once. + assertThat(tools.jdwp_resume()).contains("worker-0"); + assertThat(other.suspendCount()).isZero(); + assertThat(holder.suspendCount()).isZero(); + assertThat(sessionState()).as("the evaluation still runs").contains("leases=[worker-0]"); + final long before = calls(); + await("worker-1 to make progress meanwhile", () -> { + try { + return calls() > before; + } catch (Exception e) { + return false; + } + }); + + await("the late result", () -> history.getRecent(EventHistory.MAX_EVENTS).stream() + .anyMatch(event -> event.type().equals("EVALUATION_COMPLETED_LATE") && event.summary().contains("42"))); + assertApplicationRuns(); + } finally { + tools.jdwp_clear_breakpoint(stopId); + } + } + + @Test + @Order(6) + @DisplayName("the watchdog cancels a queued thread stop; the thread's next stop keeps its own suspension (#47 § 4)") + void cancelledThreadStopThenANewStop() throws Exception { + final VirtualMachine vm = connection.getVM(); + final ThreadReference evaluating = thread("worker-0"); + final ThreadReference holder = thread("lock-holder"); + // A read of `base` on worker-0 runs an evaluation that waits a second, then takes LOCK. + int logpointId = idOf(tools.jdwp_set_field_logpoint(TARGET, "base", "access", + "{ java.util.concurrent.locks.LockSupport.parkNanos(1_000_000_000L); synchronized (" + TARGET + + ".LOCK) { return \"got the lock\"; } }", + null, evaluating.uniqueID(), null, null, null, null, null)); + int holdingId = -1; + try { + await("an evaluation in flight on worker-0", () -> sessionState().contains("leases=[worker-0]")); + // Just this one: worker-0 waits at its event until the evaluation ends, so no later read + // raises another. + tools.jdwp_clear_breakpoint(logpointId); + logpointId = -1; + + // The holder stops inside its synchronized block, on every pass: a thread stop Q queued behind + // the evaluation, which then waits for LOCK. The watchdog cancels Q; the holder runs on, leaves + // LOCK, and stops there again — R. + final BreakpointRequest holding = vm.eventRequestManager().createBreakpointRequest( + vm.classesByName(TARGET).get(0).locationsOfLine(lineOf("// MARK: holding")).get(0)); + holding.addThreadFilter(holder); + holding.setSuspendPolicy(EventRequest.SUSPEND_EVENT_THREAD); + holdingId = session.tracker.registerBreakpoint(holding); + enableSuspended(holding, holder); + + await("the watchdog to cancel the holder's stop", () -> history.getRecent(EventHistory.MAX_EVENTS).stream() + .anyMatch(event -> event.type().equals("EVENT_CANCELLED") && event.summary().contains("lock-holder"))); + await("the evaluation to get the lock", () -> history.getRecent(EventHistory.MAX_EVENTS).stream() + .anyMatch(event -> event.type().equals("FIELD_LOGPOINT") && event.summary().contains("got the lock"))); + + final String shown = tools.jdwp_resume_until_event((int) TIMEOUT.toMillis()); + assertThat(shown).contains("Thread: lock-holder").contains("suspended=true"); + assertThat(holder.suspendCount()).as("R's own suspension, not spent by the cancelled Q").isEqualTo(1); + } finally { + if (holdingId >= 0) { + tools.jdwp_clear_breakpoint(holdingId); + } + if (logpointId >= 0) { + tools.jdwp_clear_breakpoint(logpointId); + } + } + tools.jdwp_resume(); + await("the dispatcher to finish its sets", this::dispatcherIdle); + assertApplicationRuns(); + } + + @Test + @Order(7) @DisplayName("an invocation that does not return, then disconnect: the server lets go and the application runs on (#47 § 4)") void disconnectDuringAnInvocation() throws Exception { final ThreadReference evaluating = thread("worker-1");