Skip to content

Commit 0887616

Browse files
committed
Fix isClosed() always false and stale currentState after container close
- close(): set _closed=true directly instead of calling super.close(), which would unconditionally close the underlying stream regardless of AUTO_CLOSE_TARGET - endCurrentContainer(): reset currentState to IN_ROOT when closing the root container, preventing stale state if the generator is reused Both bugs exist in msgpack-jackson; documented in plans/preexisting-issues.md
1 parent ffc69db commit 0887616

3 files changed

Lines changed: 63 additions & 15 deletions

File tree

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

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -339,6 +339,7 @@ private void endCurrentContainer()
339339
if (currentParentElementIndex == 0) {
340340
isElementsClosed = true;
341341
currentParentElementIndex = parent.parentIndex;
342+
currentState = IN_ROOT;
342343
return;
343344
}
344345

@@ -881,8 +882,6 @@ public void close() throws JacksonException
881882
{
882883
try {
883884
flush();
884-
}
885-
finally {
886885
if (StreamWriteFeature.AUTO_CLOSE_TARGET.enabledIn(_streamWriteFeatures)) {
887886
try {
888887
MessagePacker messagePacker = getMessagePacker();
@@ -893,6 +892,10 @@ public void close() throws JacksonException
893892
}
894893
}
895894
}
895+
finally {
896+
_closed = true;
897+
_releaseBuffers();
898+
}
896899
}
897900

898901
@Override

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

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -957,6 +957,45 @@ public String getName()
957957
}
958958
}
959959

960+
@Test
961+
public void testIsClosedAfterClose()
962+
throws IOException
963+
{
964+
ByteArrayOutputStream baos = new ByteArrayOutputStream();
965+
JsonGenerator generator = factory.createGenerator(baos, JsonEncoding.UTF8);
966+
assertFalse(generator.isClosed());
967+
generator.writeStartArray();
968+
generator.writeEndArray();
969+
generator.close();
970+
assertTrue(generator.isClosed());
971+
}
972+
973+
@Test
974+
public void testGeneratorReusableAfterRootContainerClose()
975+
throws IOException
976+
{
977+
ByteArrayOutputStream baos = new ByteArrayOutputStream();
978+
JsonGenerator generator = factory.createGenerator(baos, JsonEncoding.UTF8);
979+
generator.writeStartArray();
980+
generator.writeNumber(1);
981+
generator.writeEndArray();
982+
generator.flush();
983+
984+
// Write a second root value; currentState must have reset to IN_ROOT
985+
generator.writeStartObject();
986+
generator.writeName("k");
987+
generator.writeNumber(2);
988+
generator.writeEndObject();
989+
generator.close();
990+
991+
MessageUnpacker unpacker = MessagePack.newDefaultUnpacker(baos.toByteArray());
992+
assertEquals(1, unpacker.unpackArrayHeader());
993+
assertEquals(1, unpacker.unpackInt());
994+
assertEquals(1, unpacker.unpackMapHeader());
995+
assertEquals("k", unpacker.unpackString());
996+
assertEquals(2, unpacker.unpackInt());
997+
}
998+
960999
@Test
9611000
public void testWriteStringCharArrayWithOffset()
9621001
throws IOException

plans/preexisting-issues.md

Lines changed: 19 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,13 @@
22

33
## msgpack-jackson3-specific
44

5-
### 1. `isClosed()` always returns false
5+
### 1. `isClosed()` always returns false — FIXED
66

77
**File:** `msgpack-jackson3/.../MessagePackGenerator.java` (close method)
88

9-
`close()` cannot call `super.close()` because `GeneratorBase.close()` in Jackson 3
10-
closes the underlying output stream as a side effect, breaking tests that disable
11-
`AUTO_CLOSE_TARGET`. As a result `isClosed()` always returns false and callers can
12-
continue writing into a closed generator without getting the standard exception.
9+
Fixed by setting `_closed = true` directly in the `finally` block of `close()`, without
10+
calling `super.close()` (which would unconditionally close the underlying stream via
11+
`_closeInput()`, ignoring the `AUTO_CLOSE_TARGET` flag).
1312

1413
### 2. `MessagePackFactory.snapshot()` returns `this` — FIXED
1514

@@ -65,13 +64,16 @@ left off instead of from the beginning.
6564
retains the entire last parsed payload for each thread in a pool indefinitely, which
6665
can cause unbounded memory retention after large messages.
6766

68-
## 3. `MessagePackGenerator`: `close()` does not call `super.close()`
67+
## 3. `MessagePackGenerator`: `close()` does not set `isClosed()` to true
6968

70-
**File:** `msgpack-jackson/src/main/java/org/msgpack/jackson/dataformat/MessagePackGenerator.java` (close method)
69+
**Files:**
70+
- `msgpack-jackson/src/main/java/org/msgpack/jackson/dataformat/MessagePackGenerator.java` (close method)
71+
- `msgpack-jackson3/src/main/java/org/msgpack/jackson/dataformat/MessagePackGenerator.java` — FIXED
7172

72-
The `close()` override never delegates to `GeneratorBase.close()`, so `isClosed()`
73-
remains false after close. Callers can continue writing into a closed generator
74-
instead of getting the standard closed-generator exception.
73+
The `close()` override never sets the closed flag, so `isClosed()` remains false.
74+
Fixed in msgpack-jackson3 by setting `_closed = true` directly (calling `super.close()`
75+
is not viable since it unconditionally closes the underlying stream, ignoring
76+
`AUTO_CLOSE_TARGET`). Needs the same fix in msgpack-jackson.
7577

7678
## 4. `MessagePackGenerator`: `writeString(Reader, int)` crashes on length -1
7779

@@ -90,10 +92,14 @@ should try integer parsing first.
9092

9193
## 6. `MessagePackGenerator`: Closing a container leaves stale `currentState`
9294

93-
**File:** `msgpack-jackson/src/main/java/org/msgpack/jackson/dataformat/MessagePackGenerator.java` (writeEndArray/writeEndObject)
95+
**Files:**
96+
- `msgpack-jackson/src/main/java/org/msgpack/jackson/dataformat/MessagePackGenerator.java` (writeEndArray/writeEndObject)
97+
- `msgpack-jackson3/src/main/java/org/msgpack/jackson/dataformat/MessagePackGenerator.java` — FIXED
9498

95-
After `flush()` clears `nodes`, any subsequent root-level value written with the
96-
same generator is treated as if inside the old container, which can corrupt output.
99+
After closing the root container, `currentState` was not reset to `IN_ROOT`. After
100+
`flush()` clears `nodes`, any subsequent root-level value was treated as if inside the
101+
old container. Fixed in msgpack-jackson3 by adding `currentState = IN_ROOT` in
102+
`endCurrentContainer()`; needs the same fix in msgpack-jackson.
97103

98104
## 7. `MessagePackSerializedString`: Most interface methods are stubs
99105

0 commit comments

Comments
 (0)