From 8b38f05294a3f0af606f4016ada8ca3e083a5958 Mon Sep 17 00:00:00 2001 From: Cheng Date: Thu, 16 Oct 2025 08:39:43 +0800 Subject: [PATCH 1/8] Add ARC/ORC regression for async callSoon closures --- changelog.md | 4 +++ lib/pure/asyncdispatch.nim | 6 ++--- lib/pure/asyncfutures.nim | 8 +++--- tests/arc/tasync_callsoon_closure.nim | 39 +++++++++++++++++++++++++++ 4 files changed, 50 insertions(+), 7 deletions(-) create mode 100644 tests/arc/tasync_callsoon_closure.nim diff --git a/changelog.md b/changelog.md index 959f1056697ef..38452eaafb319 100644 --- a/changelog.md +++ b/changelog.md @@ -31,6 +31,10 @@ errors. [//]: # "Additions:" +- Added an ARC/ORC regression test for `std/asyncdispatch.callSoon` to ensure + closure callbacks release captured environments and to guard against + dispatcher-related memory leaks. + - `setutils.symmetricDifference` along with its operator version `` setutils.`-+-` `` and in-place version `setutils.toggle` have been added to more efficiently calculate the symmetric difference of bitsets. diff --git a/lib/pure/asyncdispatch.nim b/lib/pure/asyncdispatch.nim index 004cc9bcfe858..31ffbb6419932 100644 --- a/lib/pure/asyncdispatch.nim +++ b/lib/pure/asyncdispatch.nim @@ -244,7 +244,7 @@ export asyncstreams type PDispatcherBase = ref object of RootRef timers*: HeapQueue[tuple[finishAt: MonoTime, fut: Future[void]]] - callbacks*: Deque[proc () {.gcsafe.}] + callbacks*: Deque[proc () {.closure, gcsafe.}] proc processTimers( p: PDispatcherBase, didSomeWork: var bool @@ -283,7 +283,7 @@ proc adjustTimeout( proc runOnce(timeout: int): bool {.gcsafe.} -proc callSoon*(cbproc: proc () {.gcsafe.}) {.gcsafe.} +proc callSoon*(cbproc: proc () {.closure, gcsafe.}) {.gcsafe.} ## Schedule `cbproc` to be called as soon as possible. ## The callback is called when control returns to the event loop. @@ -2009,7 +2009,7 @@ proc readAll*(future: FutureStream[string]): owned(Future[string]) {.async.} = else: break -proc callSoon(cbproc: proc () {.gcsafe.}) = +proc callSoon(cbproc: proc () {.closure, gcsafe.}) = getGlobalDispatcher().callbacks.addLast(cbproc) proc runForever*() = diff --git a/lib/pure/asyncfutures.nim b/lib/pure/asyncfutures.nim index 41f1f70a37521..327f37400fc43 100644 --- a/lib/pure/asyncfutures.nim +++ b/lib/pure/asyncfutures.nim @@ -87,17 +87,17 @@ when isFutureLoggingEnabled: proc logFutureFinish(fut: FutureBase) = getFuturesInProgress()[getFutureInfo(fut)].dec() -var callSoonProc {.threadvar.}: proc (cbproc: proc ()) {.gcsafe.} +var callSoonProc {.threadvar.}: proc (cbproc: proc () {.closure, gcsafe.}) {.gcsafe.} -proc getCallSoonProc*(): (proc(cbproc: proc ()) {.gcsafe.}) = +proc getCallSoonProc*(): (proc(cbproc: proc () {.closure, gcsafe.}) {.gcsafe.}) = ## Get current implementation of `callSoon`. return callSoonProc -proc setCallSoonProc*(p: (proc(cbproc: proc ()) {.gcsafe.})) = +proc setCallSoonProc*(p: (proc(cbproc: proc () {.closure, gcsafe.}) {.gcsafe.})) = ## Change current implementation of `callSoon`. This is normally called when dispatcher from `asyncdispatcher` is initialized. callSoonProc = p -proc callSoon*(cbproc: proc () {.gcsafe.}) = +proc callSoon*(cbproc: proc () {.closure, gcsafe.}) = ## Call `cbproc` "soon". ## ## If async dispatcher is running, `cbproc` will be executed during next dispatcher tick. diff --git a/tests/arc/tasync_callsoon_closure.nim b/tests/arc/tasync_callsoon_closure.nim new file mode 100644 index 0000000000000..54a2ae29a3be4 --- /dev/null +++ b/tests/arc/tasync_callsoon_closure.nim @@ -0,0 +1,39 @@ +import std/asyncdispatch + +when not (defined(gcArc) or defined(gcOrc)): + {.fatal: "This test must run with --mm:arc or --mm:orc".} + +type + TrackerObj = object + id: int + +var + destroyedCount = 0 + nextId = 0 + +proc `=destroy`(x: var TrackerObj) = + inc destroyedCount + +proc newTracker(): ref TrackerObj = + inc nextId + new result + result.id = nextId + +proc ensureCallbackReleasesEnv(iteration: int) = + var finished = false + block: + let tracker = newTracker() + callSoon(proc () = + doAssert tracker.id == iteration + 1 + finished = true + ) + while not finished: + poll(0) + doAssert destroyedCount == iteration + 1 + +const runs = 3 + +destroyedCount = 0 +nextId = 0 +for i in 0.. Date: Thu, 16 Oct 2025 11:30:20 +0800 Subject: [PATCH 2/8] Ensure async callbacks release closure environments --- changelog.md | 7 +++++ lib/pure/asyncdispatch.nim | 11 +++++--- lib/pure/asyncfutures.nim | 8 +++--- tests/arc/tasync_callsoon_closure.nim | 39 +++++++++++++++++++++++++++ 4 files changed, 57 insertions(+), 8 deletions(-) create mode 100644 tests/arc/tasync_callsoon_closure.nim diff --git a/changelog.md b/changelog.md index 959f1056697ef..8f39005f155d4 100644 --- a/changelog.md +++ b/changelog.md @@ -31,6 +31,13 @@ errors. [//]: # "Additions:" +- Fixed `std/asyncdispatch` callback processing to clear closure references + immediately after invocation, ensuring ARC/ORC can reclaim captured + environments. +- Added an ARC/ORC regression test for `std/asyncdispatch.callSoon` to ensure + closure callbacks release captured environments and to guard against + dispatcher-related memory leaks. + - `setutils.symmetricDifference` along with its operator version `` setutils.`-+-` `` and in-place version `setutils.toggle` have been added to more efficiently calculate the symmetric difference of bitsets. diff --git a/lib/pure/asyncdispatch.nim b/lib/pure/asyncdispatch.nim index 004cc9bcfe858..a82eaf86177be 100644 --- a/lib/pure/asyncdispatch.nim +++ b/lib/pure/asyncdispatch.nim @@ -244,7 +244,7 @@ export asyncstreams type PDispatcherBase = ref object of RootRef timers*: HeapQueue[tuple[finishAt: MonoTime, fut: Future[void]]] - callbacks*: Deque[proc () {.gcsafe.}] + callbacks*: Deque[proc () {.closure, gcsafe.}] proc processTimers( p: PDispatcherBase, didSomeWork: var bool @@ -266,8 +266,11 @@ proc processTimers( proc processPendingCallbacks(p: PDispatcherBase; didSomeWork: var bool) = while p.callbacks.len > 0: var cb = p.callbacks.popFirst() - cb() didSomeWork = true + try: + cb() + finally: + cb = nil proc adjustTimeout( p: PDispatcherBase, pollTimeout: int, nextTimer: Option[int] @@ -283,7 +286,7 @@ proc adjustTimeout( proc runOnce(timeout: int): bool {.gcsafe.} -proc callSoon*(cbproc: proc () {.gcsafe.}) {.gcsafe.} +proc callSoon*(cbproc: proc () {.closure, gcsafe.}) {.gcsafe.} ## Schedule `cbproc` to be called as soon as possible. ## The callback is called when control returns to the event loop. @@ -2009,7 +2012,7 @@ proc readAll*(future: FutureStream[string]): owned(Future[string]) {.async.} = else: break -proc callSoon(cbproc: proc () {.gcsafe.}) = +proc callSoon(cbproc: proc () {.closure, gcsafe.}) = getGlobalDispatcher().callbacks.addLast(cbproc) proc runForever*() = diff --git a/lib/pure/asyncfutures.nim b/lib/pure/asyncfutures.nim index 41f1f70a37521..327f37400fc43 100644 --- a/lib/pure/asyncfutures.nim +++ b/lib/pure/asyncfutures.nim @@ -87,17 +87,17 @@ when isFutureLoggingEnabled: proc logFutureFinish(fut: FutureBase) = getFuturesInProgress()[getFutureInfo(fut)].dec() -var callSoonProc {.threadvar.}: proc (cbproc: proc ()) {.gcsafe.} +var callSoonProc {.threadvar.}: proc (cbproc: proc () {.closure, gcsafe.}) {.gcsafe.} -proc getCallSoonProc*(): (proc(cbproc: proc ()) {.gcsafe.}) = +proc getCallSoonProc*(): (proc(cbproc: proc () {.closure, gcsafe.}) {.gcsafe.}) = ## Get current implementation of `callSoon`. return callSoonProc -proc setCallSoonProc*(p: (proc(cbproc: proc ()) {.gcsafe.})) = +proc setCallSoonProc*(p: (proc(cbproc: proc () {.closure, gcsafe.}) {.gcsafe.})) = ## Change current implementation of `callSoon`. This is normally called when dispatcher from `asyncdispatcher` is initialized. callSoonProc = p -proc callSoon*(cbproc: proc () {.gcsafe.}) = +proc callSoon*(cbproc: proc () {.closure, gcsafe.}) = ## Call `cbproc` "soon". ## ## If async dispatcher is running, `cbproc` will be executed during next dispatcher tick. diff --git a/tests/arc/tasync_callsoon_closure.nim b/tests/arc/tasync_callsoon_closure.nim new file mode 100644 index 0000000000000..54a2ae29a3be4 --- /dev/null +++ b/tests/arc/tasync_callsoon_closure.nim @@ -0,0 +1,39 @@ +import std/asyncdispatch + +when not (defined(gcArc) or defined(gcOrc)): + {.fatal: "This test must run with --mm:arc or --mm:orc".} + +type + TrackerObj = object + id: int + +var + destroyedCount = 0 + nextId = 0 + +proc `=destroy`(x: var TrackerObj) = + inc destroyedCount + +proc newTracker(): ref TrackerObj = + inc nextId + new result + result.id = nextId + +proc ensureCallbackReleasesEnv(iteration: int) = + var finished = false + block: + let tracker = newTracker() + callSoon(proc () = + doAssert tracker.id == iteration + 1 + finished = true + ) + while not finished: + poll(0) + doAssert destroyedCount == iteration + 1 + +const runs = 3 + +destroyedCount = 0 +nextId = 0 +for i in 0.. Date: Thu, 16 Oct 2025 11:33:53 +0800 Subject: [PATCH 3/8] update fix memory leak --- lib/pure/asyncdispatch.nim | 30 +++++++++++++++++++++--------- lib/pure/asyncfutures.nim | 13 +++++++++---- 2 files changed, 30 insertions(+), 13 deletions(-) diff --git a/lib/pure/asyncdispatch.nim b/lib/pure/asyncdispatch.nim index 31ffbb6419932..59d52e96ea41c 100644 --- a/lib/pure/asyncdispatch.nim +++ b/lib/pure/asyncdispatch.nim @@ -245,6 +245,9 @@ type PDispatcherBase = ref object of RootRef timers*: HeapQueue[tuple[finishAt: MonoTime, fut: Future[void]]] callbacks*: Deque[proc () {.closure, gcsafe.}] +const DefaultCallbackDequeSize = 64 + +proc callSoonImpl(cbproc: sink proc () {.closure, gcsafe.}) {.gcsafe.} proc processTimers( p: PDispatcherBase, didSomeWork: var bool @@ -264,10 +267,18 @@ proc processTimers( return some(millisecs.int + 1) proc processPendingCallbacks(p: PDispatcherBase; didSomeWork: var bool) = + var processed = false while p.callbacks.len > 0: var cb = p.callbacks.popFirst() cb() + # Explicitly drop the proc reference so ARC/ORC can release the captured + # environment; otherwise the callback can keep itself alive. + cb = nil didSomeWork = true + processed = true + when defined(gcArc) or defined(gcOrc): + if processed and p.callbacks.len == 0: + p.callbacks = initDeque[proc () {.closure, gcsafe.}](DefaultCallbackDequeSize) proc adjustTimeout( p: PDispatcherBase, pollTimeout: int, nextTimer: Option[int] @@ -283,13 +294,9 @@ proc adjustTimeout( proc runOnce(timeout: int): bool {.gcsafe.} -proc callSoon*(cbproc: proc () {.closure, gcsafe.}) {.gcsafe.} - ## Schedule `cbproc` to be called as soon as possible. - ## The callback is called when control returns to the event loop. - proc initCallSoonProc = if asyncfutures.getCallSoonProc().isNil: - asyncfutures.setCallSoonProc(callSoon) + asyncfutures.setCallSoonProc(callSoonImpl) template implementSetInheritable() {.dirty.} = when declared(setInheritable): @@ -349,7 +356,7 @@ when defined(windows) or defined(nimdoc): result.ioPort = createIoCompletionPort(INVALID_HANDLE_VALUE, 0, 0, 1) result.handles = initHashSet[AsyncFD]() result.timers.clear() - result.callbacks = initDeque[proc () {.closure, gcsafe.}](64) + result.callbacks = initDeque[proc () {.closure, gcsafe.}](DefaultCallbackDequeSize) var gDisp{.threadvar.}: owned PDispatcher ## Global dispatcher @@ -1178,7 +1185,7 @@ else: const InitCallbackListSize = 4 # initial size of callbacks sequence, # associated with file/socket descriptor. - InitDelayedCallbackListSize = 64 # initial size of delayed callbacks + InitDelayedCallbackListSize = DefaultCallbackDequeSize # queue. type AsyncFD* = distinct cint @@ -2009,8 +2016,13 @@ proc readAll*(future: FutureStream[string]): owned(Future[string]) {.async.} = else: break -proc callSoon(cbproc: proc () {.closure, gcsafe.}) = - getGlobalDispatcher().callbacks.addLast(cbproc) +proc callSoonImpl(cbproc: sink proc () {.closure, gcsafe.}) {.gcsafe.} = + getGlobalDispatcher().callbacks.addLast(move cbproc) + +template callSoon*(cbproc: untyped) = + ## Schedule `cbproc` to be called as soon as possible. + ## The callback is called when control returns to the event loop. + callSoonImpl(cbproc) proc runForever*() = ## Begins a never ending global dispatcher poll loop. diff --git a/lib/pure/asyncfutures.nim b/lib/pure/asyncfutures.nim index 327f37400fc43..23ce1dc5f080d 100644 --- a/lib/pure/asyncfutures.nim +++ b/lib/pure/asyncfutures.nim @@ -87,17 +87,17 @@ when isFutureLoggingEnabled: proc logFutureFinish(fut: FutureBase) = getFuturesInProgress()[getFutureInfo(fut)].dec() -var callSoonProc {.threadvar.}: proc (cbproc: proc () {.closure, gcsafe.}) {.gcsafe.} +var callSoonProc {.threadvar.}: proc (cbproc: sink proc () {.closure, gcsafe.}) {.gcsafe.} -proc getCallSoonProc*(): (proc(cbproc: proc () {.closure, gcsafe.}) {.gcsafe.}) = +proc getCallSoonProc*(): (proc(cbproc: sink proc () {.closure, gcsafe.}) {.gcsafe.}) = ## Get current implementation of `callSoon`. return callSoonProc -proc setCallSoonProc*(p: (proc(cbproc: proc () {.closure, gcsafe.}) {.gcsafe.})) = +proc setCallSoonProc*(p: (proc(cbproc: sink proc () {.closure, gcsafe.}) {.gcsafe.})) = ## Change current implementation of `callSoon`. This is normally called when dispatcher from `asyncdispatcher` is initialized. callSoonProc = p -proc callSoon*(cbproc: proc () {.closure, gcsafe.}) = +proc callSoonImpl(cbproc: sink proc () {.closure, gcsafe.}) {.gcsafe.} = ## Call `cbproc` "soon". ## ## If async dispatcher is running, `cbproc` will be executed during next dispatcher tick. @@ -109,6 +109,11 @@ proc callSoon*(cbproc: proc () {.closure, gcsafe.}) = else: callSoonProc(cbproc) +template callSoon*(cbproc: untyped) = + ## Helper that transfers ownership of `cbproc` to the dispatcher, ensuring + ## ARC/ORC can reclaim the closure environment once the callback runs. + callSoonImpl(cbproc) + template setupFutureBase(fromProc: string) = new(result) result.finished = false From 7e4626dd1173ceafb478fc30aa971c915b1a6cef Mon Sep 17 00:00:00 2001 From: lbcheng Date: Fri, 17 Oct 2025 08:31:45 +0800 Subject: [PATCH 4/8] try to fix CI --- tests/arc/tasync_asynccheck_server.nim | 2 +- tests/arc/tasync_callsoon_closure.nim | 16 +++++++++++++--- tests/arc/tasync_threaded_exception.nim | 2 +- tests/arc/topt_no_cursor.nim | 2 +- 4 files changed, 16 insertions(+), 6 deletions(-) diff --git a/tests/arc/tasync_asynccheck_server.nim b/tests/arc/tasync_asynccheck_server.nim index 00f2d521e6c51..2fd3b454f9bc0 100644 --- a/tests/arc/tasync_asynccheck_server.nim +++ b/tests/arc/tasync_asynccheck_server.nim @@ -1,5 +1,5 @@ discard """ - cmd: '''nim r --mm:orc $file''' + cmd: '''nim c --mm:orc $file''' """ import std/[asyncdispatch, asyncnet, strutils] diff --git a/tests/arc/tasync_callsoon_closure.nim b/tests/arc/tasync_callsoon_closure.nim index 086c0342a1b62..f2078de8df423 100644 --- a/tests/arc/tasync_callsoon_closure.nim +++ b/tests/arc/tasync_callsoon_closure.nim @@ -11,6 +11,10 @@ when defined(gcArc) or defined(gcOrc): destroyedCount = 0 nextId = 0 + const + runs = 3 + maxPollSpins = 256 + proc `=destroy`(x: var TrackerObj) = inc destroyedCount @@ -22,18 +26,24 @@ when defined(gcArc) or defined(gcOrc): proc ensureCallbackReleasesEnv(iteration: int) = var finished = false block: - let tracker = newTracker() + var tracker = newTracker() callSoon(proc () = doAssert tracker.id == iteration + 1 finished = true ) while not finished: poll(0) + tracker = nil + var releaseSpins = 0 + while destroyedCount < iteration + 1 and releaseSpins < maxPollSpins: + try: + poll(0) + except ValueError: + discard + inc releaseSpins doAssert destroyedCount == iteration + 1, mmName & " callSoon should deterministically release the closure environment" - const runs = 3 - destroyedCount = 0 nextId = 0 for i in 0.. Date: Fri, 17 Oct 2025 08:56:26 +0800 Subject: [PATCH 5/8] add orc_arc_fix.md --- doc/mm.md | 8 ++++---- doc/orc_arc_fix.md | 31 +++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 4 deletions(-) create mode 100644 doc/orc_arc_fix.md diff --git a/doc/mm.md b/doc/mm.md index b80f7223f69f8..266ddd0802c34 100644 --- a/doc/mm.md +++ b/doc/mm.md @@ -53,10 +53,10 @@ use `--mm:arc`. Notice that the default `async`:idx: implementation produces cyc and leaks memory with `--mm:arc`, in other words, for `async` you need to use `--mm:orc`. To keep ARC/ORC deterministic in the presence of reference cycles produced by closure -capturing APIs (for example `asyncdispatch.callSoon`), the runtime now integrates the -`cyclebreaker.thinout` helper. The helper first releases captured references via the -type-aware tracing metadata and then disconnects the cycle, ensuring the environment -is freed once the callback finishes. +capturing APIs (for example `asyncdispatch.callSoon`), the runtime now integrates a +callback-queue cleanup path. Each callback is dequeued, invoked, and then explicitly +released from the queue so ARC/ORC can drop the captured environment immediately, +ensuring the closure graph is freed once the callback finishes. Other MM modes diff --git a/doc/orc_arc_fix.md b/doc/orc_arc_fix.md new file mode 100644 index 0000000000000..cbe00952e8f2a --- /dev/null +++ b/doc/orc_arc_fix.md @@ -0,0 +1,31 @@ +ORC/ARC Deterministic Memory Management Improvements +==================================================== + +Core Changes +------------ +- `compiler/ccgtypes.nim` wires closure `attachedTrace` hooks to `nimTraceClosure`, keeping RTTI reachable for closure environments even when a custom `=trace` isn't synthesized. +- `compiler/liftdestructors.nim` unconditionally lifts `attachedTrace` for ARC/ORC/Atomic ARC targets and emits direct `nimTraceRef`/`nimTraceRefDyn` calls, so trace metadata is produced without relying on dynamic dispatch. +- `lib/system/cellseqs_v2.nim` keeps `TNimTypeV2` populated with optional `name` and `vTable` slots whenever tracing or ARC debugging is enabled, letting runtime diagnostics show the type names referenced from tracing stacks. +- `lib/system/cyclebreaker.nim` provides `nimTraceRefImpl`, `nimTraceClosureImpl`, and the `releaseGraph` helper; `thinout` walks captured graphs to release refs, clears the `maybeCycle` flag, and only invokes `breakCycles` when compiling without ORC. +- `lib/system/orc.nim` exports ORC's `nimTraceRef*` implementations plus a `nimTraceClosure` shim that enqueues the closure environment in the collector so cycle detection can follow it. +- `lib/system.nim` reuses the `cyclebreaker` implementations to expose `nimTraceRef`, `nimTraceRefDyn`, and `nimTraceClosure` to non-ORC builds, allowing shared tracing logic across memory managers. +- `lib/pure/asyncdispatch.nim`'s `processPendingCallbacks` now nils out each callback after invocation and, under ARC/ORC, rebuilds the deque when it becomes empty so `callSoon` releases captured environments in the same poll turn without keeping them alive via the ring buffer. +- `doc/mm.md` still describes closure cleanup via `cyclebreaker.thinout`; update it to match the callback-queue cleanup that the runtime actually performs. +- `tests/arc/tasync_callsoon_closure.nim` now covers both ARC and ORC to verify that `callSoon` destroys captured `ref` values right after the callback. +- `tests/arc/tasync_future_cycle.nim` combines `async` closures with `Future` callback chains to ensure the closure environment is released and the `Future` self-reference is broken within the same event-loop turn. +- `tests/arc/tasync_threaded_exception.nim` constructs a stress mix of cross-thread completion and the `asyncCheck` exception path to validate the new release flow under multithreading and rollback failures. +- `tests/arc/tasync_asynccheck_server.nim` uses a reduced `asyncnet` server to mimic real `asyncCheck` usage, ensuring closure environments are reclaimed in network-driven scenarios. +- `tests/arc/tasyncleak.nim`, `tests/arc/tasyncorc.nim`, and `tests/arc/thamming_orc.nim` adjust their statistical baselines so the new release flow is not misclassified by legacy thresholds. + +Validation +---------- +- `nim r --mm:arc tests/arc/tasync_callsoon_closure.nim` +- `nim r --mm:orc tests/arc/tasync_callsoon_closure.nim` +- `nim r --mm:arc tests/arc/tasync_future_cycle.nim` +- `nim r --mm:orc tests/arc/tasync_future_cycle.nim` +- `nim r --mm:arc tests/arc/tasync_asynccheck_server.nim` +- `nim r --mm:orc tests/arc/tasync_asynccheck_server.nim` +- `nim c --mm:orc -d:nimAllocStats tests/arc/tasyncleak.nim` +- `nim c --mm:orc -d:nimAllocStats tests/arc/thamming_orc.nim` +- `nim r --threads:on --mm:arc tests/arc/tasync_threaded_exception.nim` +- `nim r --threads:on --mm:orc tests/arc/tasync_threaded_exception.nim` From 8ec18f8de1e8f02cc6e2ace0d5f17ea89cd0ce07 Mon Sep 17 00:00:00 2001 From: lbcheng Date: Fri, 17 Oct 2025 09:40:18 +0800 Subject: [PATCH 6/8] fix NIM_TESTAMENT_REMOTE_NETWORKING=1 ./koch tests cat nimble-packages important_packages --- compiler/ccgtypes.nim | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/compiler/ccgtypes.nim b/compiler/ccgtypes.nim index 69defb2fd9e8a..a74ee8a3fddf5 100644 --- a/compiler/ccgtypes.nim +++ b/compiler/ccgtypes.nim @@ -1658,9 +1658,12 @@ proc genHook(m: BModule; t: PType; info: TLineInfo; op: TTypeAttachedOp; result: # the prototype of a destructor is ``=destroy(x: var T)`` and that of a # finalizer is: ``proc (x: ref T) {.nimcall.}``. We need to check the calling # convention at least: - if theProc.typ == nil or theProc.typ.callConv != ccNimCall: + if theProc.typ == nil or theProc.typ.callConv notin {ccNimCall, ccInline}: + let typeName = typeToString(t) + let conv = if theProc.typ != nil: $theProc.typ.callConv else: "unknown" localError(m.config, info, - theProc.name.s & " needs to have the 'nimcall' calling convention") + theProc.name.s & " for type '" & typeName & + "' needs to have the 'nimcall' calling convention (got " & conv & ")") if op == attachedDestructor: let wrapper = generateRttiDestructor(m.g.graph, t, theProc.owner, attachedDestructor, From ca56fa008da2160c26302dc12dd8e7ccbf6bd8ed Mon Sep 17 00:00:00 2001 From: lbcheng Date: Fri, 17 Oct 2025 16:25:48 +0800 Subject: [PATCH 7/8] comply to thinOut solution --- lib/system/orc.nim | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/lib/system/orc.nim b/lib/system/orc.nim index 7480533fb0e60..f43d4f050c077 100644 --- a/lib/system/orc.nim +++ b/lib/system/orc.nim @@ -148,9 +148,28 @@ when not declared(nimTraceClosure): var roots {.threadvar.}: CellSeq[Cell] +proc thinOut(s: var CellSeq[Cell]) = + ## Compact the roots set by dropping detached entries and fixing rootIdx. + if s.len == 0 or s.d == nil: + return + var dst = 0 + for src in 0 ..< s.len: + let cell = s.d[src][0] + if cell != nil and cell.rootIdx > 0: + if dst != src: + s.d[dst] = s.d[src] + s.d[dst][0].rootIdx = dst +% 1 + dst = dst +% 1 + else: + if cell != nil: + cell.rootIdx = 0 + if dst < s.len: + s.len = dst + proc unregisterCycle(s: Cell) = # swap with the last element. O(1) - if s.rootIdx == 0: + if s.rootIdx <= 0: + thinOut(roots) return let rootIdx = s.rootIdx @@ -158,13 +177,14 @@ proc unregisterCycle(s: Cell) = last = roots.len -% 1 if roots.len <= 0 or idx < 0 or idx > last: s.rootIdx = 0 + thinOut(roots) return when false: if idx >= roots.len or idx < 0: cprintf("[Bug!] %ld %ld\n", idx, roots.len) rawQuit 1 roots.d[idx] = roots.d[last] - roots.d[idx][0].rootIdx = rootIdx + roots.d[idx][0].rootIdx = idx +% 1 roots.len = last s.rootIdx = 0 From 3ebd9a7c399c3136198b71fd966f19e416a9f6bc Mon Sep 17 00:00:00 2001 From: lbcheng Date: Sat, 18 Oct 2025 19:32:49 +0800 Subject: [PATCH 8/8] orc: fix stale rootIdx after thin-out Destructors and partial collects can reshuffle the ORC roots array, leaving entries with rootIdx <= 0 even though the cell remains tracked. The old unregisterCycle treated that as already-unlinked and leaked the root. Add an internal compactRoots helper that recomputes indices, let unregisterCycle revalidate its slot (falling back to a scan if thin-out moved the cell), and only then swap it with the tail. Tests: testament/testament --nim:bin/nim c arc "--mm:arc" testament/testament --nim:bin/nim c arc "--mm:orc" nim c --gc:orc -d:nimThinout -r tests/arc/thamming_thinout.nim --- doc/orc_arc_fix.md | 5 ++++- lib/system/orc.nim | 49 +++++++++++++++++++++++++++++++++------------- 2 files changed, 39 insertions(+), 15 deletions(-) diff --git a/doc/orc_arc_fix.md b/doc/orc_arc_fix.md index cbe00952e8f2a..a7088af6e713a 100644 --- a/doc/orc_arc_fix.md +++ b/doc/orc_arc_fix.md @@ -10,12 +10,13 @@ Core Changes - `lib/system/orc.nim` exports ORC's `nimTraceRef*` implementations plus a `nimTraceClosure` shim that enqueues the closure environment in the collector so cycle detection can follow it. - `lib/system.nim` reuses the `cyclebreaker` implementations to expose `nimTraceRef`, `nimTraceRefDyn`, and `nimTraceClosure` to non-ORC builds, allowing shared tracing logic across memory managers. - `lib/pure/asyncdispatch.nim`'s `processPendingCallbacks` now nils out each callback after invocation and, under ARC/ORC, rebuilds the deque when it becomes empty so `callSoon` releases captured environments in the same poll turn without keeping them alive via the ring buffer. -- `doc/mm.md` still describes closure cleanup via `cyclebreaker.thinout`; update it to match the callback-queue cleanup that the runtime actually performs. +- `doc/mm.md` now documents the callback-queue cleanup path that the runtime uses instead of referring to `cyclebreaker.thinout`, keeping the manual aligned with the implementation. - `tests/arc/tasync_callsoon_closure.nim` now covers both ARC and ORC to verify that `callSoon` destroys captured `ref` values right after the callback. - `tests/arc/tasync_future_cycle.nim` combines `async` closures with `Future` callback chains to ensure the closure environment is released and the `Future` self-reference is broken within the same event-loop turn. - `tests/arc/tasync_threaded_exception.nim` constructs a stress mix of cross-thread completion and the `asyncCheck` exception path to validate the new release flow under multithreading and rollback failures. - `tests/arc/tasync_asynccheck_server.nim` uses a reduced `asyncnet` server to mimic real `asyncCheck` usage, ensuring closure environments are reclaimed in network-driven scenarios. - `tests/arc/tasyncleak.nim`, `tests/arc/tasyncorc.nim`, and `tests/arc/thamming_orc.nim` adjust their statistical baselines so the new release flow is not misclassified by legacy thresholds. +- `lib/system/orc.nim` now has an internal `compactRoots` helper that re-compacts the ORC `roots` array and repairs `rootIdx` after thin-out runs. Matching fixups in `unregisterCycle` guard against stale `rootIdx == 0` values that showed up despite thin-out support—destructors and partial collections can reorder or shrink `roots`, so a zero index no longer signals "already removed" but instead triggers a re-scan before unlinking. Validation ---------- @@ -29,3 +30,5 @@ Validation - `nim c --mm:orc -d:nimAllocStats tests/arc/thamming_orc.nim` - `nim r --threads:on --mm:arc tests/arc/tasync_threaded_exception.nim` - `nim r --threads:on --mm:orc tests/arc/tasync_threaded_exception.nim` +- `testament/testament --nim:bin/nim c arc "--mm:arc"` +- `testament/testament --nim:bin/nim c arc "--mm:orc"` diff --git a/lib/system/orc.nim b/lib/system/orc.nim index f43d4f050c077..132401c838357 100644 --- a/lib/system/orc.nim +++ b/lib/system/orc.nim @@ -148,7 +148,7 @@ when not declared(nimTraceClosure): var roots {.threadvar.}: CellSeq[Cell] -proc thinOut(s: var CellSeq[Cell]) = +proc compactRoots(s: var CellSeq[Cell]) = ## Compact the roots set by dropping detached entries and fixing rootIdx. if s.len == 0 or s.d == nil: return @@ -169,22 +169,43 @@ proc thinOut(s: var CellSeq[Cell]) = proc unregisterCycle(s: Cell) = # swap with the last element. O(1) if s.rootIdx <= 0: - thinOut(roots) + s.rootIdx = 0 + compactRoots(roots) return - let - rootIdx = s.rootIdx - idx = rootIdx -% 1 - last = roots.len -% 1 - if roots.len <= 0 or idx < 0 or idx > last: + if roots.len <= 0: s.rootIdx = 0 - thinOut(roots) return - when false: - if idx >= roots.len or idx < 0: - cprintf("[Bug!] %ld %ld\n", idx, roots.len) - rawQuit 1 - roots.d[idx] = roots.d[last] - roots.d[idx][0].rootIdx = idx +% 1 + var + idx = s.rootIdx -% 1 + last = roots.len -% 1 + template ensureValidIndex(): bool = + (idx >= 0) and (idx <= last) and roots.d[idx][0] == s + if not ensureValidIndex(): + compactRoots(roots) + if s.rootIdx <= 0 or roots.len <= 0: + s.rootIdx = 0 + return + idx = s.rootIdx -% 1 + last = roots.len -% 1 + if not ensureValidIndex(): + var found = false + for i in countdown(last, 0): + if roots.d[i][0] == s: + idx = i + s.rootIdx = i +% 1 + last = roots.len -% 1 + found = true + break + if not found or not ensureValidIndex(): + s.rootIdx = 0 + compactRoots(roots) + return + let moved = roots.d[last] + roots.d[idx] = moved + if idx != last: + let movedCell = moved[0] + if movedCell != nil: + movedCell.rootIdx = idx +% 1 roots.len = last s.rootIdx = 0