Skylark debugging protocol: only track paused or stepping threads.

- remove blaze-side thread creation/destruction hooks
- remove ListThreads, ThreadStarted, ThreadEnded events from protocol
- don't track unpaused, not stepping threads in ThreadHandler

The threading model didn't provide useful information -- in practice,
users never want to list currently-running threads, and the debug
client doesn't need to list currently-paused threads, since it receives
each ThreadPaused and ThreadContinued event, so its model is always up to date.

Not tracking thread creation/destruction greatly simplifies the API, and
reduces the blaze-side hooks -- it was also only semi-implemented, as
plenty (/most?) skylark threads were never registered.

The biggest cost to removing this is lack of nice thread names. In
practice, I think the benefits greatly outweigh this cost. If it ever
becomes a problem, there are other lighter-weight ways of communicating
thread names.

TAG_CHANGE_OK=This proto has never yet been used
TYPE_CHANGE_OK=This proto has never yet been used
PiperOrigin-RevId: 201532462

4 files changed, 136 insertions, 263 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skylarkdebug/proto/skylark_debugging.proto b/src/main/java/com/google/devtools/build/lib/skylarkdebug/proto/skylark_debugging.proto
index d4cb453e99..918a667db4 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkdebug/proto/skylark_debugging.proto
+++ b/src/main/java/com/google/devtools/build/lib/skylarkdebug/proto/skylark_debugging.proto
@@ -29,7 +29,6 @@ message DebugRequest {
 
   // The payload describes the type of the request and its arguments, if any.
   oneof payload {
-    ListThreadsRequest list_threads = 100;
     SetBreakpointsRequest set_breakpoints = 101;
     ContinueExecutionRequest continue_execution = 102;
     EvaluateRequest evaluate = 103;
@@ -39,10 +38,6 @@ message DebugRequest {
   }
 }
 
-// A request to list the threads that are currently active running Skylark code.
-message ListThreadsRequest {
-}
-
 // A request to update the breakpoints used by the debug server.
 message SetBreakpointsRequest {
   // The breakpoints that describe where the debug server should pause
@@ -50,13 +45,21 @@ message SetBreakpointsRequest {
   repeated Breakpoint breakpoint = 1;
 }
 
-// A request to continue execution on a paused thread.
+// A request to continue execution on a paused or stepping thread. (A stepping
+// thread is a thread that is running as the result of a previous
+// ContinueExecutionRequest with non-NONE stepping.)
+//
+// A paused thread will be resumed with the given stepping, unless thread_id is
+// 0. A stepping thread will continue to run with its stepping condition
+// removed, as if it were already paused.
 message ContinueExecutionRequest {
-  // The identifier of the thread to continue.
+  // The identifier of the thread to continue. The thread must be paused or
+  // stepping.
   //
-  // If this field is not set (i.e., zero), then execution of all threads will
-  // be continued. This is typically used when the debugger disconnects from the
-  // server. In this case, the stepping field is ignored.
+  // If this field is not set (i.e., zero), then all threads will be continued
+  // without stepping; the stepping field in this message will be ignored. This
+  // is typically used when the debugger disconnects from the server.
+
   int64 thread_id = 1;
 
   // Describes the stepping behavior to use when continuing execution.
@@ -109,7 +112,6 @@ message DebugEvent {
   oneof payload {
     Error error = 99;
 
-    ListThreadsResponse list_threads = 100;
     SetBreakpointsResponse set_breakpoints = 101;
     ContinueExecutionResponse continue_execution = 102;
     EvaluateResponse evaluate = 103;
@@ -117,10 +119,8 @@ message DebugEvent {
     StartDebuggingResponse start_debugging = 105;
     PauseThreadResponse pause_thread = 106;
 
-    ThreadStartedEvent thread_started = 1000;
-    ThreadEndedEvent thread_ended = 1001;
-    ThreadPausedEvent thread_paused = 1002;
-    ThreadContinuedEvent thread_continued = 1003;
+    ThreadPausedEvent thread_paused = 1001;
+    ThreadContinuedEvent thread_continued = 1002;
   }
 }
 
@@ -131,12 +131,6 @@ message Error {
   string message = 1;
 }
 
-// The response to a ListThreadsRequest.
-message ListThreadsResponse {
-  // The threads that are currently active running Skylark code.
-  repeated Thread thread = 1;
-}
-
 // The response to a SetBreakpointsRequest.
 message SetBreakpointsResponse {
 }
@@ -168,28 +162,16 @@ message StartDebuggingResponse {
 message PauseThreadResponse {
 }
 
-// An event indicating that a thread has begun executing Skylark code.
-message ThreadStartedEvent {
-  // The thread that began.
-  Thread thread = 1;
-}
-
-// An event indicating that a thread that was executed Skylark code has ended.
-message ThreadEndedEvent {
-  // The thread that ended.
-  Thread thread = 1;
-}
-
 // An event indicating that a thread was paused during execution.
 message ThreadPausedEvent {
   // The thread that was paused.
-  Thread thread = 1;
+  PausedThread thread = 1;
 }
 
 // An event indicating that a thread has continued execution after being paused.
 message ThreadContinuedEvent {
-  // The thread that continued executing.
-  Thread thread = 1;
+  // The identifier of the thread that continued executing.
+  int64 thread_id = 1;
 }
 
 // A location where the debug server will pause execution.
@@ -255,27 +237,20 @@ enum Stepping {
   OUT = 3;
 }
 
-// Information about a thread that is running Skylark code.
-message Thread {
+// Information about a paused Skylark thread.
+message PausedThread {
   // The identifier of the thread.
   int64 id = 1;
 
-  // Present if the thread is paused, providing details such as the thread's
-  // location. Not set if the thread is running.
-  ThreadPausedState thread_paused_state = 2;
-
   // A descriptive name for the thread that can be displayed in the debugger's
   // UI.
-  string name = 3;
-}
+  string name = 2;
 
-// Information about a paused thread.
-message ThreadPausedState {
-  PauseReason pause_reason = 1;
+  PauseReason pause_reason = 3;
 
   // The location in Skylark code of the next statement or expression that will
   // be executed.
-  Location location = 2;
+  Location location = 4;
 }
 
 // The reason why a thread was paused
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/DebugEventHelper.java b/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/DebugEventHelper.java
index a389569bc2..648f9ab335 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/DebugEventHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/DebugEventHelper.java
@@ -24,22 +24,18 @@ import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.Err
 import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.EvaluateResponse;
 import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.Frame;
 import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.ListFramesResponse;
-import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.ListThreadsResponse;
 import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.PauseThreadResponse;
+import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.PausedThread;
 import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.Scope;
 import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.SetBreakpointsResponse;
 import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.StartDebuggingResponse;
-import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.Thread;
 import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.ThreadContinuedEvent;
-import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.ThreadEndedEvent;
 import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.ThreadPausedEvent;
-import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.ThreadStartedEvent;
 import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.Value;
 import com.google.devtools.build.lib.syntax.DebugFrame;
 import com.google.devtools.build.lib.syntax.Debuggable.Stepping;
 import java.util.Collection;
 import java.util.LinkedHashMap;
-import java.util.List;
 import java.util.Map;
 import javax.annotation.Nullable;
 
@@ -63,13 +59,6 @@ final class DebugEventHelper {
         .build();
   }
 
-  static DebugEvent listThreadsResponse(long sequenceNumber, List<Thread> threads) {
-    return DebugEvent.newBuilder()
-        .setSequenceNumber(sequenceNumber)
-        .setListThreads(ListThreadsResponse.newBuilder().addAllThread(threads).build())
-        .build();
-  }
-
   static DebugEvent setBreakpointsResponse(long sequenceNumber) {
     return DebugEvent.newBuilder()
         .setSequenceNumber(sequenceNumber)
@@ -112,33 +101,15 @@ final class DebugEventHelper {
         .build();
   }
 
-  static DebugEvent threadStartedEvent(long threadId, String threadName) {
-    return DebugEvent.newBuilder()
-        .setThreadStarted(
-            ThreadStartedEvent.newBuilder()
-                .setThread(Thread.newBuilder().setId(threadId).setName(threadName))
-                .build())
-        .build();
-  }
-
-  static DebugEvent threadEndedEvent(long threadId, String threadName) {
-    return DebugEvent.newBuilder()
-        .setThreadEnded(
-            ThreadEndedEvent.newBuilder()
-                .setThread(Thread.newBuilder().setId(threadId).setName(threadName))
-                .build())
-        .build();
-  }
-
-  static DebugEvent threadPausedEvent(Thread thread) {
+  static DebugEvent threadPausedEvent(PausedThread thread) {
     return DebugEvent.newBuilder()
         .setThreadPaused(ThreadPausedEvent.newBuilder().setThread(thread))
         .build();
   }
 
-  static DebugEvent threadContinuedEvent(Thread thread) {
+  static DebugEvent threadContinuedEvent(long threadId) {
     return DebugEvent.newBuilder()
-        .setThreadContinued(ThreadContinuedEvent.newBuilder().setThread(thread))
+        .setThreadContinued(ThreadContinuedEvent.newBuilder().setThreadId(threadId))
         .build();
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/SkylarkDebugServer.java b/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/SkylarkDebugServer.java
index aebc81805d..1e4853ba9c 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/SkylarkDebugServer.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/SkylarkDebugServer.java
@@ -140,20 +140,6 @@ public final class SkylarkDebugServer implements DebugServer {
     return DebugAwareEval::new;
   }
 
-  @Override
-  public <T> T runWithDebugging(Environment env, String threadName, DebugCallable<T> callable)
-      throws EvalException, InterruptedException {
-    long threadId = Thread.currentThread().getId();
-    threadHandler.registerThread(threadId, threadName, env);
-    transport.postEvent(DebugEventHelper.threadStartedEvent(threadId, threadName));
-    try {
-      return callable.call();
-    } finally {
-      transport.postEvent(DebugEventHelper.threadEndedEvent(threadId, threadName));
-      threadHandler.unregisterThread(threadId);
-    }
-  }
-
   /**
    * Pauses the execution of the current thread if there are conditions that should cause it to be
    * paused, such as a breakpoint being reached.
@@ -177,8 +163,6 @@ public final class SkylarkDebugServer implements DebugServer {
         case START_DEBUGGING:
           threadHandler.resumeAllThreads();
           return DebugEventHelper.startDebuggingResponse(sequenceNumber);
-        case LIST_THREADS:
-          return listThreads(sequenceNumber);
         case LIST_FRAMES:
           return listFrames(sequenceNumber, request.getListFrames());
         case SET_BREAKPOINTS:
@@ -199,11 +183,6 @@ public final class SkylarkDebugServer implements DebugServer {
     }
   }
 
-  /** Handles a {@code ListThreadsRequest} and returns its response. */
-  private SkylarkDebuggingProtos.DebugEvent listThreads(long sequenceNumber) {
-    return DebugEventHelper.listThreadsResponse(sequenceNumber, threadHandler.listThreads());
-  }
-
   /** Handles a {@code ListFramesRequest} and returns its response. */
   private SkylarkDebuggingProtos.DebugEvent listFrames(
       long sequenceNumber, SkylarkDebuggingProtos.ListFramesRequest request)
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/ThreadHandler.java b/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/ThreadHandler.java
index 4e9f69e1e8..a7a2d6e0f4 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/ThreadHandler.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/ThreadHandler.java
@@ -22,7 +22,6 @@ import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos;
 import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.PauseReason;
-import com.google.devtools.build.lib.skylarkdebugging.SkylarkDebuggingProtos.ThreadPausedState;
 import com.google.devtools.build.lib.syntax.Debuggable;
 import com.google.devtools.build.lib.syntax.Debuggable.ReadyToPause;
 import com.google.devtools.build.lib.syntax.Environment;
@@ -38,45 +37,36 @@ import javax.annotation.concurrent.GuardedBy;
 /** Handles all thread-related state and debugging tasks. */
 final class ThreadHandler {
 
-  private static class ThreadState {
+  /** The state of a thread that is paused. */
+  private static class PausedThreadState {
     final long id;
     final String name;
     final Debuggable debuggable;
-    /** Non-null if the thread is currently paused. */
-    @Nullable volatile PausedThreadState pausedState;
-    /** Determines when execution should next be paused. Non-null if currently stepping. */
-    @Nullable volatile ReadyToPause readyToPause;
+    /** The {@link Location} where execution is currently paused. */
+    final Location location;
+    /** Used to block execution of threads */
+    final Semaphore semaphore;
 
-    ThreadState(
-        long id,
-        String name,
-        Debuggable debuggable,
-        @Nullable PausedThreadState pausedState,
-        @Nullable ReadyToPause readyToPause) {
+    PausedThreadState(long id, String name, Debuggable debuggable, Location location) {
       this.id = id;
       this.name = name;
       this.debuggable = debuggable;
-      this.pausedState = pausedState;
-      this.readyToPause = readyToPause;
+      this.location = location;
+      this.semaphore = new Semaphore(0);
     }
   }
 
-  /** Information about a paused thread. */
-  private static class PausedThreadState {
-
-    /** The reason execution was paused. */
-    final PauseReason pauseReason;
-
-    /** The {@link Location} where execution is currently paused. */
-    final Location location;
-
-    /** Used to block execution of threads */
-    final Semaphore semaphore;
+  /**
+   * The state of a thread that is stepping, i.e. currently running but expected to stop at a
+   * subsequent statement even without a breakpoint. This may include threads that have completed
+   * running while stepping, since the ThreadHandler doesn't know when a thread terminates.
+   */
+  private static class SteppingThreadState {
+    /** Determines when execution should next be paused. */
+    final ReadyToPause readyToPause;
 
-    PausedThreadState(PauseReason pauseReason, Location location) {
-      this.pauseReason = pauseReason;
-      this.location = location;
-      this.semaphore = new Semaphore(0);
+    SteppingThreadState(ReadyToPause readyToPause) {
+      this.readyToPause = readyToPause;
     }
   }
 
@@ -88,9 +78,13 @@ final class ThreadHandler {
    */
   private volatile boolean pausingAllThreads = true;
 
-  /** A map from thread identifiers to their state info. */
-  @GuardedBy("itself")
-  private final Map<Long, ThreadState> threads = new HashMap<>();
+  /** A map from identifiers of paused threads to their state info. */
+  @GuardedBy("this")
+  private final Map<Long, PausedThreadState> pausedThreads = new HashMap<>();
+
+  /** A map from identifiers of stepping threads to their state. */
+  @GuardedBy("this")
+  private final Map<Long, SteppingThreadState> steppingThreads = new HashMap<>();
 
   /** All location-based breakpoints (the only type of breakpoint currently supported). */
   private volatile ImmutableSet<SkylarkDebuggingProtos.Location> breakpoints = ImmutableSet.of();
@@ -102,52 +96,33 @@ final class ThreadHandler {
   private final ThreadLocal<Boolean> servicingEvalRequest = ThreadLocal.withInitial(() -> false);
 
   /**
-   * Threads which are set to be paused in the next checked execution step.
+   * Threads which are not paused now, but that are set to be paused in the next checked execution
+   * step as the result of a PauseThreadRequest.
    *
-   * <p>Invariant: Every thread id in this set is also in {@link #threads}, provided that we are not
-   * in a synchronized block on that map.
+   * <p>Invariant: Every thread id in this set is also in {@link #steppingThreads}, provided that we
+   * are not in a synchronized block on the class instance.
    */
   private final Set<Long> threadsToPause = ConcurrentHashMap.newKeySet();
 
-  /** Registers a Skylark thread with the {@link ThreadHandler}. */
-  void registerThread(long threadId, String threadName, Debuggable debuggable) {
-    doRegisterThread(threadId, threadName, debuggable);
-  }
-
-  private ThreadState doRegisterThread(long threadId, String threadName, Debuggable debuggable) {
-    ThreadState thread = new ThreadState(threadId, threadName, debuggable, null, null);
-    synchronized (threads) {
-      threads.put(threadId, thread);
-    }
-    return thread;
-  }
-
   /** Mark all current and future threads paused. Will take effect asynchronously. */
   void pauseAllThreads() {
-    synchronized (threads) {
-      threadsToPause.addAll(threads.keySet());
-    }
     pausingAllThreads = true;
   }
 
   /** Mark the given thread paused. Will take effect asynchronously. */
   void pauseThread(long threadId) throws DebugRequestException {
-    synchronized (threads) {
-      if (!threads.containsKey(threadId)) {
-        throw new DebugRequestException("Unknown thread: " + threadId);
+    synchronized (this) {
+      if (!steppingThreads.containsKey(threadId)) {
+        String error =
+            pausedThreads.containsKey(threadId)
+                ? "Thread is already paused"
+                : "Unknown thread: only threads which are currently stepping can be paused";
+        throw new DebugRequestException(error);
       }
       threadsToPause.add(threadId);
     }
   }
 
-  /** Called when Skylark execution for this thread is complete. */
-  void unregisterThread(long threadId) {
-    synchronized (threads) {
-      threads.remove(threadId);
-      threadsToPause.remove(threadId);
-    }
-  }
-
   void setBreakpoints(ImmutableSet<SkylarkDebuggingProtos.Location> breakpoints) {
     // all breakpoints cover the entire line, so unset the column number.
     this.breakpoints =
@@ -157,50 +132,56 @@ final class ThreadHandler {
             .collect(toImmutableSet());
   }
 
-  /** Resumes all threads. */
+  /**
+   * Resumes all threads. Any currently stepping threads have their stepping behavior cleared, so
+   * will run unconditionally.
+   */
   void resumeAllThreads() {
     threadsToPause.clear();
     pausingAllThreads = false;
-    synchronized (threads) {
-      for (ThreadState thread : threads.values()) {
+    synchronized (this) {
+      for (PausedThreadState thread : pausedThreads.values()) {
         // continue-all doesn't support stepping.
-        resumeThread(thread, SkylarkDebuggingProtos.Stepping.NONE);
+        resumePausedThread(thread, SkylarkDebuggingProtos.Stepping.NONE);
       }
+      steppingThreads.clear();
     }
   }
 
   /**
-   * Unpauses the given thread if it is currently paused. Also unsets {@link #pausingAllThreads}.
+   * Unpauses the given thread if it is currently paused. Also unsets {@link #pausingAllThreads}. If
+   * the thread is not paused, but currently stepping, it clears the stepping behavior so it will
+   * run unconditionally.
    */
   void resumeThread(long threadId, SkylarkDebuggingProtos.Stepping stepping)
       throws DebugRequestException {
-    synchronized (threads) {
-      ThreadState thread = threads.get(threadId);
-      if (thread == null) {
-        throw new DebugRequestException(String.format("Thread %s is not running.", threadId));
+    // once the user has requested any thread be resumed, don't continue pausing future threads
+    pausingAllThreads = false;
+    synchronized (this) {
+      threadsToPause.remove(threadId);
+      if (steppingThreads.remove(threadId) != null) {
+        return;
       }
-      if (thread.pausedState == null) {
-        throw new DebugRequestException(String.format("Thread %s is not paused.", threadId));
+      PausedThreadState thread = pausedThreads.get(threadId);
+      if (thread == null) {
+        throw new DebugRequestException(
+            String.format("Unknown thread %s: cannot resume.", threadId));
       }
-      resumeThread(thread, stepping);
+      resumePausedThread(thread, stepping);
     }
   }
 
-  /**
-   * Unpauses the given thread if it is currently paused. Also unsets {@link #pausingAllThreads}.
-   */
-  @GuardedBy("threads")
-  private void resumeThread(ThreadState thread, SkylarkDebuggingProtos.Stepping stepping) {
-    PausedThreadState pausedState = thread.pausedState;
-    if (pausedState == null) {
-      return;
-    }
-    // once the user has resumed any thread, don't continue pausing future threads
-    pausingAllThreads = false;
-    thread.readyToPause =
+  /** Unpauses a currently-paused thread. */
+  @GuardedBy("this")
+  private void resumePausedThread(
+      PausedThreadState thread, SkylarkDebuggingProtos.Stepping stepping) {
+    pausedThreads.remove(thread.id);
+    ReadyToPause readyToPause =
         thread.debuggable.stepControl(DebugEventHelper.convertSteppingEnum(stepping));
-    thread.pausedState = null;
-    pausedState.semaphore.release();
+    if (readyToPause != null) {
+      steppingThreads.put(thread.id, new SteppingThreadState(readyToPause));
+    }
+    thread.semaphore.release();
   }
 
   void pauseIfNecessary(Environment env, Location location, DebugServerTransport transport) {
@@ -208,56 +189,43 @@ final class ThreadHandler {
       return;
     }
     PauseReason pauseReason = shouldPauseCurrentThread(env, location);
-    if (pauseReason != null) {
-      pauseCurrentThread(env, pauseReason, location, transport);
+    if (pauseReason == null) {
+      return;
     }
-  }
-
-  ImmutableList<SkylarkDebuggingProtos.Thread> listThreads() {
-    ImmutableList.Builder<SkylarkDebuggingProtos.Thread> list = ImmutableList.builder();
-    synchronized (threads) {
-      for (ThreadState thread : threads.values()) {
-        list.add(getThreadProto(thread));
-      }
+    long threadId = Thread.currentThread().getId();
+    threadsToPause.remove(threadId);
+    synchronized (this) {
+      steppingThreads.remove(threadId);
     }
-    return list.build();
+    pauseCurrentThread(env, pauseReason, location, transport);
   }
 
   /** Handles a {@code ListFramesRequest} and returns its response. */
   ImmutableList<SkylarkDebuggingProtos.Frame> listFrames(long threadId)
       throws DebugRequestException {
-    synchronized (threads) {
-      ThreadState thread = threads.get(threadId);
+    synchronized (this) {
+      PausedThreadState thread = pausedThreads.get(threadId);
       if (thread == null) {
-        throw new DebugRequestException(String.format("Thread %s is not running.", threadId));
+        throw new DebugRequestException(
+            String.format("Thread %s is not paused or does not exist.", threadId));
       }
-      PausedThreadState pausedState = thread.pausedState;
-      if (pausedState == null) {
-        throw new DebugRequestException(String.format("Thread %s is not paused.", threadId));
-      }
-      return listFrames(thread.debuggable, pausedState);
+      return thread
+          .debuggable
+          .listFrames(thread.location)
+          .stream()
+          .map(DebugEventHelper::getFrameProto)
+          .collect(toImmutableList());
     }
   }
 
-  private static ImmutableList<SkylarkDebuggingProtos.Frame> listFrames(
-      Debuggable debuggable, PausedThreadState pausedState) {
-    return debuggable
-        .listFrames(pausedState.location)
-        .stream()
-        .map(DebugEventHelper::getFrameProto)
-        .collect(toImmutableList());
-  }
-
   SkylarkDebuggingProtos.Value evaluate(long threadId, String expression)
       throws DebugRequestException {
     Debuggable debuggable;
-    synchronized (threads) {
-      ThreadState thread = threads.get(threadId);
+    synchronized (this) {
+      PausedThreadState thread = pausedThreads.get(threadId);
       if (thread == null) {
-        throw new DebugRequestException(String.format("Thread %s is not running.", threadId));
-      }
-      if (thread.pausedState == null) {
-        throw new DebugRequestException(String.format("Thread %s is not paused.", threadId));
+        throw new DebugRequestException(
+            String.format("Thread %s is not paused or does not exist.", threadId));
       }
       debuggable = thread.debuggable;
     }
@@ -283,41 +251,25 @@ final class ThreadHandler {
       Environment env, PauseReason pauseReason, Location location, DebugServerTransport transport) {
     long threadId = Thread.currentThread().getId();
 
-    SkylarkDebuggingProtos.Thread threadProto;
-    PausedThreadState pausedState;
-    synchronized (threads) {
-      ThreadState thread = threads.get(threadId);
-      if (thread == null) {
-        // this skylark entry point didn't call DebugServer#runWithDebugging. Now that we've hit a
-        // breakpoint, register it anyway.
-        // TODO(bazel-team): once all skylark evaluation routes through
-        // DebugServer#runWithDebugging, report an error here instead
-        String fallbackThreadName = "Untracked thread: " + threadId;
-        transport.postEvent(DebugEventHelper.threadStartedEvent(threadId, fallbackThreadName));
-        thread = doRegisterThread(threadId, fallbackThreadName, env);
-      }
-      pausedState = new PausedThreadState(pauseReason, location);
-      thread.pausedState = pausedState;
-      // get proto after setting the paused state, so that it's up to date
-      threadProto = getThreadProto(thread);
+    PausedThreadState pausedState =
+        new PausedThreadState(threadId, Thread.currentThread().getName(), env, location);
+    synchronized (this) {
+      pausedThreads.put(threadId, pausedState);
     }
-
+    SkylarkDebuggingProtos.PausedThread threadProto =
+        getPausedThreadProto(pausedState, pauseReason);
     transport.postEvent(DebugEventHelper.threadPausedEvent(threadProto));
     pausedState.semaphore.acquireUninterruptibly();
-    transport.postEvent(
-        DebugEventHelper.threadContinuedEvent(
-            threadProto.toBuilder().clearThreadPausedState().build()));
+    transport.postEvent(DebugEventHelper.threadContinuedEvent(threadId));
   }
 
   @Nullable
   private PauseReason shouldPauseCurrentThread(Environment env, Location location) {
     long threadId = Thread.currentThread().getId();
-    boolean pausingAllThreads = this.pausingAllThreads;
     if (pausingAllThreads) {
-      threadsToPause.remove(threadId);
       return PauseReason.ALL_THREADS_PAUSED;
     }
-    if (threadsToPause.remove(threadId)) {
+    if (threadsToPause.contains(threadId)) {
       return PauseReason.PAUSE_THREAD_REQUEST;
     }
     if (hasBreakpointAtLocation(location)) {
@@ -326,9 +278,9 @@ final class ThreadHandler {
 
     // TODO(bazel-team): if contention becomes a problem, consider changing 'threads' to a
     // concurrent map, and synchronizing on individual entries
-    synchronized (threads) {
-      ThreadState thread = threads.get(threadId);
-      if (thread != null && thread.readyToPause != null && thread.readyToPause.test(env)) {
+    synchronized (this) {
+      SteppingThreadState steppingState = steppingThreads.get(threadId);
+      if (steppingState != null && steppingState.readyToPause.test(env)) {
         return PauseReason.STEPPING;
       }
     }
@@ -348,17 +300,13 @@ final class ThreadHandler {
   }
 
   /** Returns a {@code Thread} proto builder with information about the given thread. */
-  private static SkylarkDebuggingProtos.Thread getThreadProto(ThreadState thread) {
-    SkylarkDebuggingProtos.Thread.Builder builder =
-        SkylarkDebuggingProtos.Thread.newBuilder().setId(thread.id).setName(thread.name);
-
-    PausedThreadState pausedState = thread.pausedState;
-    if (pausedState != null) {
-      builder.setThreadPausedState(
-          ThreadPausedState.newBuilder()
-              .setPauseReason(pausedState.pauseReason)
-              .setLocation(DebugEventHelper.getLocationProto(pausedState.location)));
-    }
-    return builder.build();
+  private static SkylarkDebuggingProtos.PausedThread getPausedThreadProto(
+      PausedThreadState thread, PauseReason pauseReason) {
+    return SkylarkDebuggingProtos.PausedThread.newBuilder()
+        .setId(thread.id)
+        .setName(thread.name)
+        .setPauseReason(pauseReason)
+        .setLocation(DebugEventHelper.getLocationProto(thread.location))
+        .build();
   }
 }