Skip to content

Commit 184556e

Browse files
AlexPl292claude
andcommitted
Make StdioTransport accept Flow<String> + suspend writer
Promote the Flow-based pair to the primary constructor and keep the existing Source/Sink one as a back-compat secondary, marked @deprecated: the blocking variant pins a dispatcher thread per readLine and would saturate the I/O pool under high agent concurrency. Fix close() to cancel childScope. Previously close() only closed the channels and invoked closeHandler; the Source/Sink path worked because closing the underlying streams unblocked readLine, but the Flow path has no equivalent unblock — close() would leave the read job parked inside input.collect and the transport stuck in CLOSING. Cancelling childScope cooperatively unwinds the collect, the read job exits, joinAll returns, and the finally block sets state to CLOSED. The Source/Sink path is unaffected (the cancel is a no-op once the job has already exited). Add StdioTransportFlowTest covering the new primary path: round-trip send/receive, JSON-skip behaviour, input-flow completion, input-flow errors, the output exception contract (IOException = clean, anything else = onError), parent-scope cancellation, and the close-deadlock regression itself. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 75a23c9 commit 184556e

3 files changed

Lines changed: 331 additions & 31 deletions

File tree

acp/api/acp.api

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -501,6 +501,8 @@ public abstract class com/agentclientprotocol/transport/BaseTransport : com/agen
501501
}
502502

503503
public final class com/agentclientprotocol/transport/StdioTransport : com/agentclientprotocol/transport/BaseTransport {
504+
public fun <init> (Lkotlinx/coroutines/CoroutineScope;Lkotlinx/coroutines/CoroutineDispatcher;Lkotlinx/coroutines/flow/Flow;Lkotlin/jvm/functions/Function2;Ljava/lang/String;)V
505+
public synthetic fun <init> (Lkotlinx/coroutines/CoroutineScope;Lkotlinx/coroutines/CoroutineDispatcher;Lkotlinx/coroutines/flow/Flow;Lkotlin/jvm/functions/Function2;Ljava/lang/String;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
504506
public fun <init> (Lkotlinx/coroutines/CoroutineScope;Lkotlinx/coroutines/CoroutineDispatcher;Lkotlinx/io/Source;Lkotlinx/io/Sink;Ljava/lang/String;)V
505507
public synthetic fun <init> (Lkotlinx/coroutines/CoroutineScope;Lkotlinx/coroutines/CoroutineDispatcher;Lkotlinx/io/Source;Lkotlinx/io/Sink;Ljava/lang/String;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
506508
public fun close ()V

acp/src/commonMain/kotlin/com/agentclientprotocol/transport/StdioTransport.kt

Lines changed: 112 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -4,12 +4,12 @@ import com.agentclientprotocol.rpc.ACPJson
44
import com.agentclientprotocol.rpc.JsonRpcMessage
55
import com.agentclientprotocol.rpc.decodeJsonRpcMessage
66
import com.agentclientprotocol.transport.Transport.State
7-
import com.agentclientprotocol.util.checkCancelled
87
import io.github.oshai.kotlinlogging.KotlinLogging
98
import kotlinx.coroutines.*
109
import kotlinx.coroutines.channels.Channel
10+
import kotlinx.coroutines.flow.Flow
11+
import kotlinx.coroutines.flow.flow
1112
import kotlinx.coroutines.flow.getAndUpdate
12-
import kotlinx.coroutines.flow.update
1313
import kotlinx.io.*
1414
import kotlinx.serialization.encodeToString
1515

@@ -21,47 +21,94 @@ private val logger = KotlinLogging.logger {}
2121
* This transport communicates over standard input/output streams,
2222
* which is commonly used for command-line agents.
2323
*/
24-
public class StdioTransport(
24+
public class StdioTransport private constructor(
2525
private val parentScope: CoroutineScope,
2626
private val ioDispatcher: CoroutineDispatcher,
27-
private val input: Source,
28-
private val output: Sink,
29-
private val name: String = StdioTransport::class.simpleName!!,
27+
private val input: Flow<String>,
28+
private val output: suspend (String) -> Unit,
29+
private val name: String,
30+
private val closeHandler: () -> Unit,
3031
) : BaseTransport() {
32+
33+
/**
34+
* Primary [Flow]-based constructor.
35+
*
36+
* @param parentScope coroutine scope for the transport's lifecycle
37+
* @param ioDispatcher dispatcher used for the read and write coroutines
38+
* @param input cold flow of incoming NDJSON lines. Cancellation of the
39+
* transport cancels collection; use [Flow.onCompletion] to react to
40+
* that cancellation if you need to release upstream resources.
41+
* @param output suspending writer invoked once per outgoing line. The
42+
* implementation owns framing (newline) and flushing semantics. To
43+
* signal that the underlying transport has closed cleanly, throw
44+
* [IllegalStateException] or [IOException]; the write loop will exit
45+
* without firing an error. Any other exception is reported via
46+
* [Transport.onError].
47+
* @param name optional name used in coroutine names and log messages
48+
*/
49+
public constructor(
50+
parentScope: CoroutineScope,
51+
ioDispatcher: CoroutineDispatcher,
52+
input: Flow<String>,
53+
output: suspend (String) -> Unit,
54+
name: String = StdioTransport::class.simpleName!!,
55+
) : this(parentScope, ioDispatcher, input, output, name, closeHandler = {})
56+
57+
/**
58+
* Back-compat constructor backed by blocking [Source] / [Sink].
59+
*
60+
* Deprecated because this variant blocks the dispatcher thread for the
61+
* duration of each [Source.readLine]. Under high agent concurrency this can
62+
* saturate the I/O dispatcher (e.g. `Dispatchers.IO`) and cascade into
63+
* freezes if other consumers schedule blocking work on the same pool.
64+
*
65+
* When this constructor is removed, the [Flow]-based constructor should be
66+
* promoted to the primary one and [closeHandler] dropped from its parameter
67+
* list (cancelling [childScope] in [close] is already enough to unwind the
68+
* Flow-based read path).
69+
*
70+
* Callers should adapt their blocking [Source] / [Sink] into a
71+
* [Flow]`<String>` and a `suspend (String) -> Unit` at the call site and
72+
* use the [Flow]-based constructor instead.
73+
*/
74+
@Deprecated(
75+
message = "Blocking Source/Sink pins a dispatcher thread per read and forces an extra closeHandler. " +
76+
"Adapt your I/O into Flow<String> + suspend (String) -> Unit and use the Flow-based constructor.",
77+
level = DeprecationLevel.WARNING,
78+
)
79+
public constructor(
80+
parentScope: CoroutineScope,
81+
ioDispatcher: CoroutineDispatcher,
82+
input: Source,
83+
output: Sink,
84+
name: String = StdioTransport::class.simpleName!!,
85+
) : this(
86+
parentScope, ioDispatcher,
87+
sourceAsLineFlow(input), sinkAsLineWriter(output),
88+
name,
89+
closeHandler = makeSourceSinkCloseHandler(input, output),
90+
)
91+
3192
private val childScope = CoroutineScope(parentScope.coroutineContext + SupervisorJob(parentScope.coroutineContext[Job]) + CoroutineName(name))
3293

3394
private val receiveChannel = Channel<JsonRpcMessage>(Channel.UNLIMITED)
3495
private val sendChannel = Channel<JsonRpcMessage>(Channel.UNLIMITED)
35-
96+
3697
override fun start() {
3798
if (_state.getAndUpdate { State.STARTING } != State.CREATED) error("Transport is not in ${State.CREATED.name} state")
3899
// Start reading messages from input
39100
childScope.launch(CoroutineName("$name.join-jobs")) {
40101
val readJob = launch(ioDispatcher + CoroutineName("$name.read-from-input")) {
41102
try {
42-
while (currentCoroutineContext().isActive) {
103+
// ACP assumes working with ND Json (new line delimited Json) when working over stdio
104+
input.collect { line ->
43105
currentCoroutineContext().ensureActive()
44-
// ACP assumes working with ND Json (new line delimited Json) when working over stdio
45-
val line = try {
46-
input.readLine()
47-
} catch (e: IllegalStateException) {
48-
logger.trace(e) { "Input stream closed" }
49-
break
50-
} catch (e: IOException) {
51-
logger.trace(e) { "Input stream likely closed" }
52-
break
53-
}
54-
if (line == null) {
55-
// End of stream
56-
logger.trace { "End of stream" }
57-
break
58-
}
59106

60107
val jsonRpcMessage = try {
61108
decodeJsonRpcMessage(line)
62109
} catch (t: Throwable) {
63110
logger.trace(t) { "Failed to decode JSON message: $line" }
64-
continue
111+
return@collect
65112
}
66113
logger.trace { "Sending message to channel: $jsonRpcMessage" }
67114
fireMessage(jsonRpcMessage)
@@ -84,9 +131,7 @@ public class StdioTransport(
84131
for (message in sendChannel) {
85132
val encoded = ACPJson.encodeToString(message)
86133
try {
87-
output.writeString(encoded)
88-
output.writeString("\n")
89-
output.flush()
134+
output(encoded)
90135
} catch (e: IllegalStateException) {
91136
logger.trace(e) { "Output stream closed" }
92137
break
@@ -129,7 +174,7 @@ public class StdioTransport(
129174
}
130175
}
131176
}
132-
177+
133178
override fun send(message: JsonRpcMessage) {
134179
logger.trace { "Sending message: $message" }
135180
val channelResult = sendChannel.trySend(message)
@@ -150,7 +195,43 @@ public class StdioTransport(
150195
if (sendChannel.close()) logger.trace { "Send channel closed" }
151196
if (receiveChannel.close()) logger.trace { "Receive channel closed" }
152197

153-
runCatching { input.close() }.onFailure { logger.warn(it) { "Exception when closing input stream" } }
154-
runCatching { output.close() }.onFailure { logger.warn(it) { "Exception when closing output stream" } }
198+
runCatching { closeHandler() }.onFailure { logger.warn(it) { "Exception in close handler" } }
199+
200+
// Unwind the read/write coroutines. The Source/Sink back-compat path relies
201+
// on [closeHandler] closing the underlying streams to unblock readLine, but
202+
// the Flow-based path has no equivalent unblock — without cancelling here
203+
// the read job would stay parked inside input.collect and the transport
204+
// would be stuck in CLOSING.
205+
childScope.cancel()
155206
}
156-
}
207+
}
208+
209+
private fun sourceAsLineFlow(source: Source): Flow<String> = flow {
210+
while (true) {
211+
val line = try {
212+
source.readLine()
213+
} catch (e: IllegalStateException) {
214+
logger.trace(e) { "Input stream closed" }
215+
break
216+
} catch (e: IOException) {
217+
logger.trace(e) { "Input stream likely closed" }
218+
break
219+
}
220+
if (line == null) {
221+
logger.trace { "End of stream" }
222+
break
223+
}
224+
emit(line)
225+
}
226+
}
227+
228+
private fun sinkAsLineWriter(sink: Sink): suspend (String) -> Unit = { line ->
229+
sink.writeString(line)
230+
sink.writeString("\n")
231+
sink.flush()
232+
}
233+
234+
private fun makeSourceSinkCloseHandler(source: Source, sink: Sink): () -> Unit = {
235+
runCatching { source.close() }.onFailure { logger.warn(it) { "Exception when closing input stream" } }
236+
runCatching { sink.close() }.onFailure { logger.warn(it) { "Exception when closing output stream" } }
237+
}

0 commit comments

Comments
 (0)