mirror of
https://github.com/nim-lang/Nim.git
synced 2026-07-31 20:49:06 +00:00
Fixes #23615 ## Root cause The leak does not require async at all -- this minimal closure iterator leaks the exception and its stacktrace seq under ARC/ORC: ```nim iterator it(): int {.closure.} = try: yield 1 # try spanning a yield => closureiters transform raise newException(ValueError, "x") except ValueError: # typed except => generated `of` check discard yield 2 ``` A bare `except:` does not leak; a *typed* `except` does: 1. `collectExceptState` in `compiler/closureiters.nim` generates the except-branch type check as `of(getCurrentException(), T)`, using the raw generic magic sym from `getSysMagic("of", mOf)`. 2. `injectdestructors` skips call arguments whose *formal* parameter type is `isCompileTimeOnly`, and the raw generic `of` sym's formal params are `tyGenericParam` so both arguments of the generated `of` call are never processed. 3. `getCurrentException()` increfs `currException` via `=copy` into its result. Since the arc pass never wraps that owned temp in a destroy (`--expandArc` shows the condition left untouched, while a user-written `if f() of ValueError` in the same iterator gets a `:tmpD` + `=destroy`), the caught exception's refcount stays +1 forever. Every `try: await x() except SomeError` in async code has this shape, so each caught async exception leaked once. ## Fix The state-machine wrapper already stores the active exception in the `:curExc` env field before jumping to the except landing state, and `currException == :curExc` on every path into that state. The generated condition now references the env field via `ctx.newCurExcAccess()` instead of calling `getCurrentException()` again -- no ownership transfer, no temp to destroy, one fewer runtime call. Note: the underlying `injectdestructors` behavior (skipping args of calls whose formal params are raw `tyGenericParam`, e.g. from `getSysMagic`) is a separate latent gap that could affect other compiler-generated code; it is intentionally left untouched here. ## Valgrind, before and after Exact code and command from the issue, on Linux (Valgrind 3.19): ``` nim c -d:danger --mm:orc --debugger:native --threads:off -d:useMalloc bug.nim valgrind --leak-check=full --show-leak-kinds=all ./bug ``` Before (devel): ``` ==14663== HEAP SUMMARY: ==14663== in use at exit: 136 bytes in 2 blocks ==14663== total heap usage: 22 allocs, 20 frees, 116,466 bytes allocated ==14663== ==14663== 56 bytes in 1 blocks are indirectly lost in loss record 1 of 2 ==14663== at 0x488A1C4: realloc (vg_replace_malloc.c:1437) ==14663== by 0x10C663: prepareSeqAddUninit (seqs_v2.nim:212) ==14663== by 0x10CF6F: raiseExceptionEx (excpt.nim:538) ==14663== by 0x11661F: amain::amainX20X28AsyncX29_(Future<void>) (bug.nim:10) ==14663== ... ==14663== ==14663== 136 (80 direct, 56 indirect) bytes in 1 blocks are definitely lost in loss record 2 of 2 ==14663== at 0x48850C8: malloc (vg_replace_malloc.c:381) ==14663== by 0x10C8E3: nimNewObj (arc.nim:122) ==14663== by 0x115403: err::errX20X28AsyncX29_(Future<void>) (asyncmacro.nim:274) ==14663== by 0x116027: err::errNimAsyncContinue(Future<void>, ClosureIt<void>) (asyncmacro.nim:44) ==14663== by 0x1163D3: bug::err (bug.nim:3) ==14663== ... ==14663== ==14663== LEAK SUMMARY: ==14663== definitely lost: 80 bytes in 1 blocks ==14663== indirectly lost: 56 bytes in 1 blocks ==14663== possibly lost: 0 bytes in 0 blocks ==14663== still reachable: 0 bytes in 0 blocks ==14663== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0) ``` After (this PR): ``` ==14675== HEAP SUMMARY: ==14675== in use at exit: 0 bytes in 0 blocks ==14675== total heap usage: 22 allocs, 22 frees, 116,466 bytes allocated ==14675== ==14675== All heap blocks were freed -- no leaks are possible ==14675== ==14675== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0) ``` ## Testing - New `tests/async/t23615.nim` (modeled on `t23212.nim`: `valgrind: true` + alloc-stats assertion) covers both the pure closure-iterator form and the async form from the issue, with the caught exception looped 50x so the leak blows well past the slack threshold. It passes with this PR and fails against devel. - Testament categories `async`, `arc`, `iter`, `exception` all pass with the patched compiler (323 tests). - Behavior is unchanged on a sanity program covering multi-branch dispatch, `as e` binding, nested try, and re-raise across yields: output is byte-identical to devel; the patched build just frees 2 more blocks per caught exception.
This commit is contained in:
@@ -336,9 +336,14 @@ proc collectExceptState(ctx: var Ctx, n: PNode): PNode {.inline.} =
|
||||
var cond: PNode = nil
|
||||
for i in 0..<c.len - 1:
|
||||
assert(c[i].kind == nkType)
|
||||
# Use the :curExc env field (set by the wrapper before entering the
|
||||
# except landing state) instead of calling getCurrentException():
|
||||
# injectdestructors does not process the args of this raw generic
|
||||
# `of` magic call, so an owning getCurrentException() temp would
|
||||
# never be destroyed and the caught exception would leak (#23615).
|
||||
let nextCond = newTreeIT(nkCall, c.info, ctx.g.getSysType(c.info, tyBool),
|
||||
newSymNode(g.getSysMagic(c.info, "of", mOf)),
|
||||
g.callCodegenProc("getCurrentException"),
|
||||
ctx.newCurExcAccess(),
|
||||
c[i])
|
||||
|
||||
cond = if cond.isNil: nextCond
|
||||
|
||||
52
tests/async/t23615.nim
Normal file
52
tests/async/t23615.nim
Normal file
@@ -0,0 +1,52 @@
|
||||
discard """
|
||||
valgrind: true
|
||||
cmd: '''nim c --mm:orc -d:nimAllocStats -d:useMalloc $file'''
|
||||
output: '''ok'''
|
||||
"""
|
||||
|
||||
# bug #23615: exceptions caught by a typed except branch in a closure
|
||||
# iterator (and thus in any async proc) leaked under ARC/ORC.
|
||||
|
||||
import std/[asyncdispatch, importutils]
|
||||
|
||||
privateAccess(AllocStats)
|
||||
|
||||
block: # pure closure iterator, the minimal form of the bug
|
||||
proc runIter() =
|
||||
iterator it(): int {.closure.} =
|
||||
try:
|
||||
yield 1
|
||||
raise newException(ValueError, "x")
|
||||
except ValueError:
|
||||
discard
|
||||
yield 2
|
||||
var f = it
|
||||
doAssert f() == 1
|
||||
doAssert f() == 2
|
||||
let base = getAllocStats()
|
||||
runIter()
|
||||
GC_fullCollect()
|
||||
let after = getAllocStats()
|
||||
doAssert after.allocCount - after.deallocCount ==
|
||||
base.allocCount - base.deallocCount, $base & " " & $after
|
||||
|
||||
block: # the async incarnation from the issue
|
||||
proc err {.async.} =
|
||||
raise newException(ValueError, "err1")
|
||||
|
||||
proc amain {.async.} =
|
||||
await sleepAsync(1)
|
||||
for _ in 0..<50:
|
||||
try:
|
||||
await err()
|
||||
except ValueError:
|
||||
discard
|
||||
|
||||
waitFor amain()
|
||||
doAssert not hasPendingOperations()
|
||||
setGlobalDispatcher(nil)
|
||||
GC_fullCollect()
|
||||
|
||||
let stats = getAllocStats()
|
||||
doAssert stats.allocCount - stats.deallocCount < 10, $stats
|
||||
echo "ok"
|
||||
Reference in New Issue
Block a user