Skip to content

Commit 6f5c34e

Browse files
dividedmindclaude
andcommitted
fix: Close underlying Writers in RecordingSession to prevent resource leaks
The fastjson JSONWriter.close() method does not close the underlying Writer passed to its constructor - it only closes the internal SerializeWriter, which has no effect. This caused FileOutputStream and OutputStreamWriter instances to leak in RecordingSession. Changes: - Make AppMapSerializer implement AutoCloseable and track the underlying Writer to close it explicitly - Add try-with-resources for AppMapSerializer in checkpoint() to ensure proper cleanup on exceptions - Add try-finally in stop() to ensure serializer is closed even if write() fails - Consolidate JSON finalization and closing logic into close() method, removing the redundant finish() method Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
1 parent 64fcc6b commit 6f5c34e

File tree

2 files changed

+50
-25
lines changed

2 files changed

+50
-25
lines changed

agent/src/main/java/com/appland/appmap/record/AppMapSerializer.java

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
/**
1818
* Writes AppMap data to JSON.
1919
*/
20-
public class AppMapSerializer {
20+
public class AppMapSerializer implements AutoCloseable {
2121
public static class FileSections {
2222
public static final String Version = "version";
2323
public static final String Metadata = "metadata";
@@ -37,10 +37,13 @@ private class SectionInfo {
3737
}
3838

3939
private final JSONWriter json;
40+
private final Writer underlyingWriter;
4041
private SectionInfo currentSection = null;
4142
private final HashSet<String> sectionsWritten = new HashSet<String>();
43+
private boolean closed = false;
4244

4345
private AppMapSerializer(Writer writer) {
46+
this.underlyingWriter = writer;
4447
this.json = new JSONWriter(writer);
4548
// The eventUpdates object contains Event objects that are also in the
4649
// events array. Setting DisableCircularReferenceDetect tells fastjson that
@@ -73,7 +76,7 @@ public void write(CodeObjectTree classMap, Metadata metadata, Map<Integer, Event
7376
writeMetadata(git, metadata);
7477
}
7578
writeEventUpdates(eventUpdates);
76-
finish();
79+
close();
7780
}
7881

7982
private void setCurrentSection(String section, String type) throws IOException {
@@ -280,12 +283,22 @@ private void writeEventUpdates(Map<Integer, Event> updates) throws IOException {
280283
}
281284

282285
/**
283-
* Closes outstanding JSON objects and closes the writer.
286+
* Closes outstanding JSON objects and closes the underlying writer. Safe to call multiple times.
284287
* @throws IOException If a writer error occurs
285288
*/
286-
private void finish() throws IOException {
287-
this.setCurrentSection("EOF", "");
288-
this.json.endObject();
289-
this.json.close();
289+
@Override
290+
public void close() throws IOException {
291+
if (!this.closed) {
292+
try {
293+
this.setCurrentSection("EOF", "");
294+
this.json.endObject();
295+
this.json.close();
296+
} finally {
297+
// JSONWriter.close() does not close the underlying writer, so we must do it explicitly
298+
// Always close the underlying writer, even if JSON finalization fails
299+
this.underlyingWriter.close();
300+
this.closed = true;
301+
}
302+
}
290303
}
291304
}

agent/src/main/java/com/appland/appmap/record/RecordingSession.java

Lines changed: 30 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -93,22 +93,24 @@ public synchronized Recording checkpoint() {
9393
// By using RandomAccessFile we can erase that character.
9494
// If we don't let the JSON writer write the "begin object" token, it refuses
9595
// to do anything else properly either.
96-
RandomAccessFile raf = new RandomAccessFile(targetPath.toFile(), "rw");
97-
Writer fw = new OutputStreamWriter(new OutputStream() {
98-
@Override
99-
public void write(int b) throws IOException {
100-
raf.write(b);
96+
try (RandomAccessFile raf = new RandomAccessFile(targetPath.toFile(), "rw")) {
97+
Writer fw = new OutputStreamWriter(new OutputStream() {
98+
@Override
99+
public void write(int b) throws IOException {
100+
raf.write(b);
101+
}
102+
}, StandardCharsets.UTF_8);
103+
raf.seek(targetPath.toFile().length());
104+
105+
if (eventReceived) {
106+
fw.write("],");
101107
}
102-
}, StandardCharsets.UTF_8);
103-
raf.seek(targetPath.toFile().length());
108+
fw.flush();
104109

105-
if ( eventReceived ) {
106-
fw.write("],");
110+
try (AppMapSerializer serializer = AppMapSerializer.reopen(fw, raf)) {
111+
serializer.write(this.getClassMap(), this.metadata, this.eventUpdates);
112+
}
107113
}
108-
fw.flush();
109-
110-
AppMapSerializer serializer = AppMapSerializer.reopen(fw, raf);
111-
serializer.write(this.getClassMap(), this.metadata, this.eventUpdates);
112114
} catch (IOException e) {
113115
throw new RuntimeException(e);
114116
}
@@ -124,16 +126,24 @@ public synchronized Recording stop() {
124126
throw new IllegalStateException("AppMap: Unable to stop the recording because no recording is in progress.");
125127
}
126128

129+
File file = this.tmpPath.toFile();
127130
try {
128131
this.serializer.write(this.getClassMap(), this.metadata, this.eventUpdates);
129132
} catch (IOException e) {
130133
throw new RuntimeException(e);
134+
} finally {
135+
// Ensure serializer is closed even if write() throws an exception
136+
try {
137+
if (this.serializer != null) {
138+
this.serializer.close();
139+
}
140+
} catch (IOException e) {
141+
logger.error("Failed to close serializer", e);
142+
}
143+
this.serializer = null;
144+
this.tmpPath = null;
131145
}
132146

133-
File file = this.tmpPath.toFile();
134-
this.serializer = null;
135-
this.tmpPath = null;
136-
137147
logger.debug("Recording finished");
138148
logger.debug("Wrote recording to file {}", file.getPath());
139149

@@ -163,7 +173,9 @@ void start() {
163173
try {
164174
this.tmpPath = Files.createTempFile(null, ".appmap.json");
165175
this.tmpPath.toFile().deleteOnExit();
166-
this.serializer = AppMapSerializer.open(new OutputStreamWriter(new FileOutputStream(this.tmpPath.toFile()), StandardCharsets.UTF_8));
176+
FileOutputStream fileOutputStream = new FileOutputStream(this.tmpPath.toFile());
177+
OutputStreamWriter writer = new OutputStreamWriter(fileOutputStream, StandardCharsets.UTF_8);
178+
this.serializer = AppMapSerializer.open(writer);
167179
} catch (IOException e) {
168180
this.tmpPath = null;
169181
this.serializer = null;

0 commit comments

Comments
 (0)