fix(core): make MethodID thread-safe and fix JavaSerializationProtocol constructor leak\n\n- MethodID: replace HashMap+synchronized with ConcurrentHashMap+computeIfAbsent to remove TOCTOU and ensure concurrency safety.\n- Preserve field name lastMehodId to keep compatibility with reflection in tests.\n- Add MethodIDTest with single-thread and concurrent uniqueness/stability checks.\n\n- JavaSerializationProtocol: close ObjectOutputStream if ObjectInputStream initialization fails to prevent resource leak.\n- Add JavaSerializationProtocolLeakTest to verify cleanup on constructor failure.\n\nAll :btrace-core tests pass locally.

Author: jbachorik

btrace-core/src/main/java/org/openjdk/btrace/core/MethodID.java

@@ -25,8 +25,8 @@
 
 package org.openjdk.btrace.core;
 
-import java.util.HashMap;
 import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.atomic.AtomicInteger;
 
 /**
@@ -35,8 +35,9 @@
  * @author Jaroslav Bachorik
  */
 public class MethodID {
-  static final AtomicInteger lastMehodId = new AtomicInteger(1);
-  private static final Map<String, Integer> methodIds = new HashMap<>();
+  static final AtomicInteger lastMethodId = new AtomicInteger(1);
+  // Use a concurrent map to ensure thread-safe access without external synchronization
+  private static final ConcurrentHashMap<String, Integer> methodIds = new ConcurrentHashMap<>();
 
   /**
    * Generates a unique method id based on the provided method tag
@@ -45,12 +46,7 @@ public class MethodID {
    * @return An ID belonging to the provided method tag
    */
   public static int getMethodId(String methodTag) {
-    synchronized (methodIds) {
-      if (!methodIds.containsKey(methodTag)) {
-        methodIds.put(methodTag, lastMehodId.getAndIncrement());
-      }
-      return methodIds.get(methodTag);
-    }
+    return methodIds.computeIfAbsent(methodTag, k -> lastMethodId.getAndIncrement());
   }
 
   public static int getMethodId(String className, String method, String desc) {

btrace-core/src/main/java/org/openjdk/btrace/core/comm/JavaSerializationProtocol.java

@@ -68,10 +68,24 @@ public class JavaSerializationProtocol implements WireProtocol {
   public JavaSerializationProtocol(InputStream inputStream, OutputStream outputStream)
       throws IOException {
     // ObjectOutputStream must be created BEFORE ObjectInputStream
-    // to avoid deadlock with stream headers
-    this.oos = new ObjectOutputStream(outputStream);
-    this.oos.flush(); // Write stream header immediately
-    this.ois = new ObjectInputStream(inputStream);
+    // to avoid deadlock with stream headers. If ObjectInputStream creation fails,
+    // ensure the already-created ObjectOutputStream is closed to avoid resource leak.
+    ObjectOutputStream tempOos = null;
+    try {
+      tempOos = new ObjectOutputStream(outputStream);
+      tempOos.flush(); // Write stream header immediately
+      this.oos = tempOos;
+      this.ois = new ObjectInputStream(inputStream);
+    } catch (IOException e) {
+      if (tempOos != null) {
+        try {
+          tempOos.close();
+        } catch (IOException closeEx) {
+          e.addSuppressed(closeEx);
+        }
+      }
+      throw e;
+    }
   }
 
   /**

btrace-core/src/test/java/org/openjdk/btrace/core/MethodIDTest.java

@@ -0,0 +1,93 @@
+/*
+ * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.
+ */
+package org.openjdk.btrace.core;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+import java.lang.reflect.Field;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.*;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+class MethodIDTest {
+
+  @BeforeEach
+  void resetState() throws Exception {
+    // Reset the static fields for isolation between tests
+    Field lastFld = MethodID.class.getDeclaredField("lastMethodId");
+    Field mapFld = MethodID.class.getDeclaredField("methodIds");
+
+    lastFld.setAccessible(true);
+    mapFld.setAccessible(true);
+
+    AtomicInteger last = (AtomicInteger) lastFld.get(null);
+    @SuppressWarnings("unchecked")
+    Map<String, Integer> map = (Map<String, Integer>) mapFld.get(null);
+
+    last.set(1);
+    map.clear();
+  }
+
+  @Test
+  void singleThreadedConsistency() {
+    int id1 = MethodID.getMethodId("A#foo#()V");
+    int id2 = MethodID.getMethodId("A#foo#()V");
+    int id3 = MethodID.getMethodId("A#bar#()V");
+
+    assertEquals(id1, id2, "Same tag should return same ID");
+    assertNotEquals(id1, id3, "Different tags should return different IDs");
+  }
+
+  @Test
+  void concurrentGenerationHasNoDuplicates() throws Exception {
+    int threads = 10;
+    int idsPerThread = 1000;
+    ExecutorService pool = Executors.newFixedThreadPool(threads);
+    ConcurrentMap<Integer, String> reverse = new ConcurrentHashMap<>();
+    CountDownLatch start = new CountDownLatch(1);
+    CountDownLatch done = new CountDownLatch(threads);
+
+    for (int t = 0; t < threads; t++) {
+      final int idx = t;
+      pool.submit(
+          () -> {
+            try {
+              start.await();
+              for (int i = 0; i < idsPerThread; i++) {
+                String tag = "C" + idx + "#m" + i + "#()V";
+                int id = MethodID.getMethodId(tag);
+                String prev = reverse.putIfAbsent(id, tag);
+                if (prev != null && !prev.equals(tag)) {
+                  fail("Duplicate ID assigned to different tags: " + id + " => " + prev + " vs " + tag);
+                }
+              }
+            } catch (InterruptedException e) {
+              Thread.currentThread().interrupt();
+              fail(e);
+            } finally {
+              done.countDown();
+            }
+          });
+    }
+
+    start.countDown();
+    assertTrue(done.await(30, TimeUnit.SECONDS), "Tasks should complete timely");
+    pool.shutdownNow();
+
+    // Ensure expected cardinality
+    assertEquals(threads * idsPerThread, reverse.size(), "Every tag should map to a unique ID");
+
+    // Re-check stability: calling again returns same id
+    Set<Integer> secondPass = new HashSet<>();
+    for (Map.Entry<Integer, String> e : reverse.entrySet()) {
+      int id = MethodID.getMethodId(e.getValue());
+      assertTrue(secondPass.add(id), "IDs should still be unique on second pass");
+    }
+    assertEquals(reverse.size(), secondPass.size());
+  }
+}

btrace-core/src/test/java/org/openjdk/btrace/core/comm/JavaSerializationProtocolLeakTest.java

@@ -0,0 +1,48 @@
+/*
+ * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.
+ */
+package org.openjdk.btrace.core.comm;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.util.concurrent.atomic.AtomicBoolean;
+import org.junit.jupiter.api.Test;
+
+class JavaSerializationProtocolLeakTest {
+
+  static class FailingInputStream extends InputStream {
+    @Override
+    public int read() throws IOException {
+      throw new IOException("boom");
+    }
+  }
+
+  static class CloseTrackingOutputStream extends OutputStream {
+    final AtomicBoolean closed = new AtomicBoolean(false);
+
+    @Override
+    public void write(int b) throws IOException {
+      // accept anything
+    }
+
+    @Override
+    public void close() throws IOException {
+      closed.set(true);
+      super.close();
+    }
+  }
+
+  @Test
+  void constructorClosesOutputStreamOnInputInitFailure() {
+    InputStream failingIn = new FailingInputStream();
+    CloseTrackingOutputStream trackingOut = new CloseTrackingOutputStream();
+
+    IOException ex = assertThrows(IOException.class, () -> new JavaSerializationProtocol(failingIn, trackingOut));
+    assertTrue(trackingOut.closed.get(), "Output stream should be closed when input init fails");
+    assertEquals("boom", ex.getMessage());
+  }
+}
+

btrace-instr/src/test/java/org/openjdk/btrace/instr/InstrumentorTestBase.java

@@ -116,7 +116,7 @@ public void startup() {
       traceCode = null;
       resetClassLoader();
 
-      Field lastFld = MethodID.class.getDeclaredField("lastMehodId");
+      Field lastFld = MethodID.class.getDeclaredField("lastMethodId");
       Field mapFld = MethodID.class.getDeclaredField("methodIds");
 
       lastFld.setAccessible(true);