Fix five issues flagged in second AI code review

- MessagePackParser: always reset unpacker for byte[] sources (|| src instanceof byte[])
  to remove subtle dependency on the ThreadLocal-clearing fix; regression test added
- MessagePackGenerator: use bb.duplicate().get(data) for non-array-backed ByteBuffers
  to avoid advancing the caller's buffer position
- MessagePackGenerator: release OutputStream reference in _releaseBuffers() via
  messageBufferOutput.reset(null), avoiding retention without the 23% regression
  that messageBufferOutputHolder.remove() caused
- MessagePackExtensionType: add toString() so extension-type map keys appear
  meaningfully in read-context error messages
- preexisting-issues.md: document fixes and add issue #12 (ByteBuffer non-array path)

Author: komamitsu

msgpack-jackson3/src/main/java/org/msgpack/jackson/dataformat/MessagePackExtensionType.java

@@ -55,6 +55,12 @@ public int hashCode()
         return result;
     }
 
+    @Override
+    public String toString()
+    {
+        return "MessagePackExtensionType(type=" + type + ", data.length=" + (data == null ? 0 : data.length) + ")";
+    }
+
     public static class Serializer extends StdSerializer<MessagePackExtensionType>
     {
         public Serializer()

msgpack-jackson3/src/main/java/org/msgpack/jackson/dataformat/MessagePackGenerator.java

@@ -400,7 +400,7 @@ else if (v instanceof ByteBuffer) {
             }
             else {
                 byte[] data = new byte[len];
-                bb.get(data);
+                bb.duplicate().get(data);
                 messagePacker.packBinaryHeader(len);
                 messagePacker.addPayload(data);
             }
@@ -965,6 +965,15 @@ protected void _closeInput() throws IOException
     @Override
     protected void _releaseBuffers()
     {
+        OutputStreamBufferOutput messageBufferOutput = messageBufferOutputHolder.get();
+        if (messageBufferOutput != null) {
+            try {
+                messageBufferOutput.reset(null);
+            }
+            catch (IOException e) {
+                throw _wrapIOFailure(e);
+            }
+        }
     }
 
     @Override

msgpack-jackson3/src/main/java/org/msgpack/jackson/dataformat/MessagePackParser.java

@@ -105,7 +105,7 @@ public MessagePackParser(
             messageUnpacker = MessagePack.newDefaultUnpacker(input);
         }
         else {
-            if (StreamReadFeature.AUTO_CLOSE_SOURCE.enabledIn(streamReadFeatures) || messageUnpackerTuple.first() != src) {
+            if (StreamReadFeature.AUTO_CLOSE_SOURCE.enabledIn(streamReadFeatures) || messageUnpackerTuple.first() != src || src instanceof byte[]) {
                 messageUnpackerTuple.second().reset(input);
             }
             messageUnpacker = messageUnpackerTuple.second();

msgpack-jackson3/src/test/java/org/msgpack/jackson/dataformat/MessagePackParserTest.java

@@ -1108,4 +1108,25 @@ public void testByteArrayThreadLocalClearedAfterClose()
         List<Integer> result = objectMapper.readValue(bytes, new TypeReference<List<Integer>>() {});
         assertEquals(Arrays.asList(1, 2, 3), result);
     }
+
+    @Test
+    public void testByteArrayReuseResetsUnpackerWhenAutoCloseSourceDisabled()
+            throws IOException
+    {
+        ObjectMapper objectMapper = MessagePackMapper.builder(new MessagePackFactory())
+                .disable(tools.jackson.core.StreamReadFeature.AUTO_CLOSE_SOURCE)
+                .build();
+
+        byte[] bytes = objectMapper.writeValueAsBytes(Arrays.asList(1, 2, 3));
+
+        // First parse succeeds
+        List<Integer> first = objectMapper.readValue(bytes, new TypeReference<List<Integer>>() {});
+        assertEquals(Arrays.asList(1, 2, 3), first);
+
+        // Second parse with the same byte[] instance and AUTO_CLOSE_SOURCE disabled.
+        // The unpacker is not reset (src identity match, no AUTO_CLOSE_SOURCE trigger),
+        // so it continues from EOF and fails to produce the expected result.
+        List<Integer> second = objectMapper.readValue(bytes, new TypeReference<List<Integer>>() {});
+        assertEquals(Arrays.asList(1, 2, 3), second);
+    }
 }

plans/preexisting-issues.md

@@ -54,12 +54,11 @@ Also fixed in the same pass:
   never reset after the first `writeName`, making every subsequent `writeName` return
   false.
 
-Note: `messageBufferOutputHolder` ThreadLocal was NOT fixed here — calling
-`messageBufferOutputHolder.remove()` in `_releaseBuffers()` caused a 23% serialization
-regression by defeating the ThreadLocal caching (each close allocates a new
-`OutputStreamBufferOutput` on the next generator creation). Dropped in favour of
-accepting the minor OutputStream retention, which is only observable when a thread
-creates exactly one generator and never creates another.
+Note: `messageBufferOutputHolder` ThreadLocal OutputStream retention — FIXED (see
+preexisting-issues section 4 below). `messageBufferOutputHolder.remove()` caused a 23%
+regression; instead `_releaseBuffers()` now calls `messageBufferOutput.reset(null)` to
+release the OutputStream reference while keeping the `OutputStreamBufferOutput` object
+alive for reuse.
 
 ### 5. `writeString(Reader, int)` len≥0 path allocates unbounded buffer — FIXED
 
@@ -143,12 +142,27 @@ identically in msgpack-jackson. They should be addressed in both modules togethe
 
 ## 1. `MessagePackParser`: Same byte-array input skips unpacker reset
 
-**File:** `msgpack-jackson/src/main/java/org/msgpack/jackson/dataformat/MessagePackParser.java:130`
+**Files:**
+- `msgpack-jackson/src/main/java/org/msgpack/jackson/dataformat/MessagePackParser.java:130`
+- `msgpack-jackson3/src/main/java/org/msgpack/jackson/dataformat/MessagePackParser.java:108` — FIXED
+
+When `AUTO_CLOSE_SOURCE` is disabled and the same `byte[]` instance is parsed more
+than once, the condition `messageUnpackerTuple.first() != src` is false, so the unpacker
+is not reset. The second parse continues from where the first left off (typically EOF)
+instead of from the beginning.
+
+Bug only manifests when all three conditions hold: (1) `reuseResourceInParser=true`,
+(2) `AUTO_CLOSE_SOURCE` disabled (non-default), (3) the same `byte[]` reference reused.
+
+**Note on msgpack-jackson3:** In practice, issue #2's fix (clearing the `byte[]` from the
+ThreadLocal on `close()`) incidentally prevents this bug from manifesting — after close,
+the cached src becomes `null`, so the next parse always sees `null != bytes` and resets.
+Fixed explicitly anyway (`|| src instanceof byte[]`) to make intent clear and remove the
+subtle dependency between the two fixes. Regression test:
+`MessagePackParserTest.testByteArrayReuseResetsUnpackerWhenAutoCloseSourceDisabled`.
 
-When `AUTO_CLOSE_SOURCE` is disabled and the same byte-array instance is parsed more
-than once (e.g. reused buffer), the condition `messageUnpackerTuple.first() != src`
-is false, so the unpacker is not reset. The second parse continues from where the first
-left off instead of from the beginning.
+Needs the same explicit fix in msgpack-jackson (where issue #2 is also not yet fixed, so
+both bugs compound).
 
 ## 2. `MessagePackParser`: ThreadLocal retains last byte-array payload per thread
 
@@ -259,3 +273,35 @@ Fixed in msgpack-jackson3 using `bb.arrayOffset() + bb.position()`; needs the sa
 with a `ByteBuffer` field that was sliced or partially consumed will silently produce
 corrupt serialized output. No exception is thrown.
 
+## 11. `MessagePackFactory`: byte-array parser copies the input slice unnecessarily
+
+**Files:**
+- `msgpack-jackson/src/main/java/org/msgpack/jackson/dataformat/MessagePackFactory.java:143`
+- `msgpack-jackson3/src/main/java/org/msgpack/jackson/dataformat/MessagePackFactory.java` — FIXED
+
+`_createParser(byte[], int, int)` calls `Arrays.copyOfRange(data, offset, offset + len)` to
+extract the slice, then wraps the copy in `InputStreamBufferInput`. `ArrayBufferInput` already
+accepts `(data, offset, len)` directly and avoids the copy entirely.
+Fixed in msgpack-jackson3 by constructing `ArrayBufferInput(data, offset, len)` and passing it
+to the `MessageBufferInput`-taking constructor; needs the same fix in msgpack-jackson.
+
+**Practical impact:** Low for small payloads; proportional to payload size for large messages
+since the entire slice is copied on every `readValue` call.
+
+## 12. `MessagePackGenerator`: non-array-backed ByteBuffer read advances caller's position
+
+**Files:**
+- `msgpack-jackson/src/main/java/org/msgpack/jackson/dataformat/MessagePackGenerator.java:373`
+- `msgpack-jackson3/src/main/java/org/msgpack/jackson/dataformat/MessagePackGenerator.java` — FIXED
+
+When serializing a `ByteBuffer` whose `hasArray()` is false (e.g. direct buffers), the
+generator calls `bb.get(data)` which advances the buffer's position as a side effect. The
+`hasArray()` fast path is non-destructive (`writePayload(bb.array(), bb.arrayOffset() +
+bb.position(), len)`), so the two paths are inconsistent.
+Fixed in msgpack-jackson3 using `bb.duplicate().get(data)` — `duplicate()` shares the
+backing store without copying data (O(1)) but has its own independent position.
+Needs the same fix in msgpack-jackson.
+
+**Practical impact:** Low. Direct ByteBuffers are uncommon in typical POJO fields, and the
+mutation is only observable if the caller inspects or reuses the buffer after serialization.
+