Skip to content

Commit e940d5c

Browse files
committed
[kyo-net] ConnectionPool: linearize release against close so a raced release cannot orphan a connection
release() and close() were not linearizable on the per-host ring. A connection returned by release at the same instant as close could land in a ring close had already drained, or in a fresh HostPool getPool re-created after close cleared the map. Nothing drains that ring again, so the connection never reaches the discard callback and its socket stays open with its read armed. On the shared io_uring transport this surfaced as a flaky end-of-run fd leak (pendingCloses=0) in the kyo-sql container suites. release now re-reads closed after publishing and drains-and-discards its HostPool when the pool closed under it. The HostPool drain, shared by close and the new drainDiscard, consumes every claimed slot up to tail and spins over a slot that is mid-publish rather than stopping at it, so a release that claimed a slot but has not yet stored its sequence is waited for. The ring head CAS keeps disposal exactly-once across both drainers. ConnectionPoolTest reproduces the orphan deterministically through a default no-op test seam and asserts the connection is disposed exactly once, plus a concurrency smoke that races n releases against one close.
1 parent 8861871 commit e940d5c

2 files changed

Lines changed: 122 additions & 18 deletions

File tree

kyo-net/shared/src/main/scala/kyo/net/internal/ConnectionPool.scala

Lines changed: 63 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,11 @@ final private[kyo] class ConnectionPool[K, C](
3333
/** True once `close()` has run. For testing the client's close/release path only. */
3434
private[kyo] def isClosed(using AllowUnsafe): Boolean = closed
3535

36+
// Test seam (default no-op): a deterministic interleaving point for the release-vs-close linearizability regression in
37+
// ConnectionPoolTest. It runs after release() has observed the pool open but before it publishes, so a test can drive
38+
// close() into exactly the window the shared-transport fd leak lives in. Never set outside that test.
39+
private[internal] var raceProbe: () => Unit = ConnectionPool.noRaceProbe
40+
3641
/** Try to get a live idle connection for the given host. */
3742
def poll(key: K)(using AllowUnsafe): Maybe[C] =
3843
if closed then Maybe.empty
@@ -41,7 +46,21 @@ final private[kyo] class ConnectionPool[K, C](
4146
/** Return a connection to the idle pool. If the ring is full, discard it. */
4247
def release(key: K, conn: C)(using AllowUnsafe): Unit =
4348
if closed then discardConn(conn)
44-
else getPool(key).release(conn, discardConn)
49+
else
50+
raceProbe()
51+
val hostPool = getPool(key)
52+
hostPool.release(conn, discardConn)
53+
// close() can race this release: it sets `closed`, drains every host pool, and clears the map, any of which may
54+
// fall between the `closed` read above and the publish just done. A connection published into a ring close()
55+
// already drained (or a fresh pool getPool re-created after pools.clear()) would otherwise never be drained again
56+
// and its socket never closed. Re-read `closed`. If it is now set, drain and discard this host pool ourselves. The
57+
// ring's head CAS makes disposal exactly-once against close()'s own drain.
58+
if closed then
59+
hostPool.drainDiscard(discardConn)
60+
// Drop the entry we may have re-created after close()'s pools.clear() so it does not linger. The two-arg remove
61+
// unmaps only this exact instance, so a fresh pool another releaser inserted for the same key is left alone.
62+
kyo.discard(pools.remove(key, hostPool))
63+
end if
4564

4665
/** Discard a connection without returning it to the pool. */
4766
def discard(conn: C)(using AllowUnsafe): Unit =
@@ -81,6 +100,9 @@ end ConnectionPool
81100

82101
private[kyo] object ConnectionPool:
83102

103+
// The shared default for `raceProbe`: a single no-op instance so a production pool allocates no per-instance lambda.
104+
private[internal] val noRaceProbe: () => Unit = () => ()
105+
84106
def init[K, C](
85107
maxConnectionsPerHost: Int,
86108
idleConnectionTimeout: Duration,
@@ -178,28 +200,51 @@ private[kyo] object ConnectionPool:
178200
def unreserve(): Unit =
179201
kyo.discard(inFlight.decrementAndGet())
180202

181-
/** Close the pool. Drains idle connections for the caller to close. */
182-
def close[C](into: ChunkBuilder[C]): Unit =
203+
/** Drain every slot claimed for release, from `head` up to `tail`, applying `sink` to each connection.
204+
*
205+
* A concurrent `release` publishes in two steps: it CASes `tail` to claim a slot, then stores the connection and
206+
* `lazySet`s the slot's sequence to mark it readable. A drain that stopped at the first slot whose sequence is not yet
207+
* visible would treat a slot being published right now as the end of the ring and leave that connection behind, never
208+
* drained again. So while `head` is below `tail` a stale sequence means a claim is mid-publish: spin until its store
209+
* lands rather than terminate. A claimer between its CAS and its store is running on its own carrier (release never
210+
* suspends), so the wait is bounded. A single-threaded runtime has no release in flight while this runs, so the spin
211+
* is never taken. Ends when `head == tail`.
212+
*/
213+
private def drainClaimed[C](sink: C => Unit): Unit =
183214
@tailrec def loop(): Unit =
184215
val currentHead = head.get()
185-
val idx = (currentHead % capacity).toInt
186-
val seq = sequences.get(idx)
187-
if seq < currentHead + 1 then ()
188-
else if head.compareAndSet(currentHead, currentHead + 1) then
189-
connections(idx) match
190-
case Present(conn) =>
191-
connections(idx) = Absent
192-
kyo.discard(into += conn.asInstanceOf[C])
193-
case Absent =>
194-
connections(idx) = Absent
195-
end match
196-
sequences.lazySet(idx, currentHead + capacity)
197-
loop()
198-
else loop()
216+
val currentTail = tail.get()
217+
if currentHead >= currentTail then ()
218+
else
219+
val idx = (currentHead % capacity).toInt
220+
val seq = sequences.get(idx)
221+
if seq < currentHead + 1 then loop()
222+
else if head.compareAndSet(currentHead, currentHead + 1) then
223+
connections(idx) match
224+
case Present(conn) =>
225+
connections(idx) = Absent
226+
sink(conn.asInstanceOf[C])
227+
case Absent =>
228+
connections(idx) = Absent
229+
end match
230+
sequences.lazySet(idx, currentHead + capacity)
231+
loop()
232+
else loop()
233+
end if
199234
end if
200235
end loop
201236
loop()
202-
end close
237+
end drainClaimed
238+
239+
/** Close the pool. Drains idle connections for the caller to close. */
240+
def close[C](into: ChunkBuilder[C]): Unit =
241+
drainClaimed[C](conn => kyo.discard(into += conn))
242+
243+
/** Drain and discard every connection still in the ring, for a `release` that observed the pool closed after it had
244+
* already published. Shares [[drainClaimed]] with [[close]], so the same wait-for-a-mid-publish-claim rule applies.
245+
*/
246+
def drainDiscard[C](discardConn: C => Unit): Unit =
247+
drainClaimed(discardConn)
203248

204249
end HostPool
205250

kyo-net/shared/src/test/scala/kyo/net/internal/ConnectionPoolTest.scala

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import kyo.net.Test
88
class ConnectionPoolTest extends Test:
99

1010
import AllowUnsafe.embrace.danger
11+
given Frame = Frame.internal
1112

1213
val key1 = NetAddress.Tcp("host1", 80)
1314
val key2 = NetAddress.Tcp("host2", 80)
@@ -133,4 +134,62 @@ class ConnectionPoolTest extends Test:
133134
assert(discardCount.get() == 2)
134135
}
135136

137+
"release that observes close mid-publish disposes the connection, never orphans it (fd-leak race regression, CI #1837)" in {
138+
// The shared-transport fd leak: release(key, conn) passes its `closed` check, then close() runs (drains every host
139+
// pool, sets closed, clears the map). Release then re-creates a host pool via computeIfAbsent and publishes into a
140+
// ring nothing else will ever drain, so the connection's socket is never closed (the CI dump: pendingCloses=0, recv
141+
// still armed). The raceProbe seam fires close() in exactly that window, so the interleaving is deterministic on
142+
// every platform. The connection must still be disposed exactly once.
143+
val discardCount = AtomicInt.Unsafe.init(0)
144+
val pool =
145+
ConnectionPool.init[NetAddress, String](2, kyo.Duration.Infinity, _ => true, _ => discard(discardCount.incrementAndGet()))
146+
pool.raceProbe = () => discard(pool.close())
147+
pool.release(key1, "c")
148+
assert(
149+
discardCount.get() == 1,
150+
s"the connection must be disposed exactly once (1); got ${discardCount.get()} (0 = orphaned/leaked)"
151+
)
152+
}
153+
154+
"close drains concurrently with releases without orphaning or double-disposing (concurrency smoke)" in {
155+
// The linearizable-drain path under real preemption: `n` concurrent releases race one close on a ring sized to hold
156+
// them, so close catches slots mid-publish and its drain must spin over them rather than stop. Every released
157+
// connection ends up extracted by close or discarded, exactly once. This also guards the drain's spin against
158+
// deadlock under contention. JS has no in-method preemption, so it passes by construction.
159+
val n = 32
160+
val scenarios = 1000
161+
val expected = (0 until n).map("c" + _).toSet
162+
Loop(0) { s =>
163+
if s >= scenarios then Loop.done(assert(true))
164+
else
165+
val discarded = AtomicRef.Unsafe.init(Chunk.empty[String])
166+
val pool =
167+
ConnectionPool.init[NetAddress, String](
168+
n,
169+
kyo.Duration.Infinity,
170+
_ => true,
171+
c => discard(discarded.updateAndGet(_ :+ c))
172+
)
173+
for
174+
latch <- Latch.init(1)
175+
releasers <- Fiber.init(Async.foreach(0 until n, n)(j => latch.await.map(_ => Sync.defer(pool.release(key1, "c" + j)))))
176+
closer <- Fiber.init(latch.await.map(_ => Sync.defer(pool.close())))
177+
_ <- latch.release
178+
_ <- releasers.get
179+
extracted <- closer.get
180+
yield
181+
// Exactly-once per connection: every released id lands in exactly one of the two sets, none orphaned, none doubled.
182+
val ext = extracted.toArray.toSet
183+
val dis = discarded.get().toArray.toSet
184+
if (ext ++ dis) == expected && ext.intersect(dis).isEmpty then Loop.continue(s + 1)
185+
else
186+
Loop.done(assert(
187+
(ext ++ dis) == expected && ext.intersect(dis).isEmpty,
188+
s"scenario $s: exactly-once violated. orphaned=${expected -- ext -- dis}, double-disposed=${ext.intersect(dis)}"
189+
))
190+
end if
191+
end for
192+
}
193+
}
194+
136195
end ConnectionPoolTest

0 commit comments

Comments
 (0)