Skip to content

use fixed array of ctx to avoid consuming all fds#14

Merged
Ivansete-status merged 11 commits into
masterfrom
fix-leak-fd
May 12, 2026
Merged

use fixed array of ctx to avoid consuming all fds#14
Ivansete-status merged 11 commits into
masterfrom
fix-leak-fd

Conversation

@Ivansete-status

@Ivansete-status Ivansete-status commented May 1, 2026

Copy link
Copy Markdown
Collaborator

Use fixed array of 32 context to allow reuse them and hence, prevent app from running out of file descriptors.
Notice that this is mostly needed for testing purposes. For example, we detected an issue when dealing with Go unit tests that started/stopped several contexts, without recycling, and then the test process run out of FDs.

closes #4

p.s.: there are some other minor changes like enhancing the error handling, use of wait signal in watchdog to allow fast shutdown (and make tests faster.)

Comment thread ffi/ffi_context.nim Outdated
Comment thread ffi/ffi_context.nim Outdated
Comment thread ffi/ffi_context.nim Outdated
Comment thread ffi/ffi_context.nim Outdated
Comment on lines +234 to +238
proc cleanUpResources[T](ctx: ptr FFIContext[T]): Result[void, string] =
## Full cleanup for heap-allocated contexts: closes all resources and frees memory.
defer:
freeShared(ctx)
return ctx.closeResources()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could have a single cleanup proc and add a freeCtx: bool = false (or default to true, if that makes more sense)

Comment thread ffi/ffi_context.nim Outdated
ctx.onNotResponding()
return err("failed to signal reqSignal on time in destroyFFIContext")

let wdSignaled = ctx.watchdogStopSignal.fireSync().valueOr:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wdSignaled is the same as above, should have the same name. I'd just called it stop or maybe stopSignaled, but call it that everywhere

Comment thread ffi/ffi_context.nim Outdated

@gmelodie gmelodie left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides the other comments, do we even use this at all?

proc destroyFFIContext[T](ctx: ptr FFIContext[T]): Result[void, string] =

I see that tests introduced all use pool.destroyFFIContext

Comment thread examples/nim_timer/nim_timer.nim Outdated
# This call is a no-op unless -d:ffiGenBindings is passed to the compiler.
genBindings() # reads -d:ffiOutputDir, -d:ffiNimSrcRelPath, -d:targetLang from compile flags
genBindings()
# reads -d:ffiOutputDir, -d:ffiNimSrcRelPath, -d:targetLang from compile flags

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe this comment should be inside the definition of genBindings? Here it may cause confusion as to what it refers to

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 397150e

)
)

let poolDecl = quote do:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL about quote!

Comment thread ffi/ffi_context.nim Outdated
Comment on lines +383 to +384
for i in 0 ..< MaxFFIContexts:
var expected = false

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for i in 0 ..< MaxFFIContexts:
var expected = false
const expected = false
for i in 0 ..< MaxFFIContexts:

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks but compareExchange in atomics requires a var:

proc compareExchange[T: not Trivial](location: var Atomic[T]; expected: var T;
                                     desired: T; order: MemoryOrder = moSequentiallyConsistent): bool

Comment thread ffi/ffi_context.nim Outdated

joinThread(ctx.ffiThread)
joinThread(ctx.watchdogThread)
ctx.cleanUpResources().isOkOr:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line seems to be the only difference between the two destroyFFIContext implementations, maybe we could wrap the rest in another function and make the two impls call the new function? e.g.

proc newFn(ctx...) =
  unregisterCtx(cast[pointer](ctx))
  ...
  joinThread(ctx.ffiThread)
  joinThread(ctx.watchdogThread)

# then ...
  
proc destroyFFIContext*[T](
    pool: var FFIContextPool[T], ctx: ptr FFIContext[T]
): Result[void, string] =
  newFn(ctx)
  pool.releaseSlot(ctx)
  return ok()

# and ...

proc destroyFFIContext[T](ctx: ptr FFIContext[T]): Result[void, string] =
  newFn(ctx)
  ctx.cleanUpResources().isOkOr:
    return err("cleanUpResources failed: " & $error)
  return ok()

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it was confusing because both had the same name but one call the other.
The b53f3ce may have enhanced that.

@NagyZoltanPeter NagyZoltanPeter left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yepp, this is needed but I see a bit of discrepancy in between we reserve slots and store the ctx pointer in there while we have another store for ctx ptrs.
I suggest a bit of rework here, eliminate that contextRegistry for which we always do a lock at every api call and just use the slot registry for the same puropse - for validating the ctx ptr - reading slot registry does not need a lock.

@Ivansete-status

Copy link
Copy Markdown
Collaborator Author

Besides the other comments, do we even use this at all?

proc destroyFFIContext[T](ctx: ptr FFIContext[T]): Result[void, string] =

@gmelodie - this proc is needed to clear the context. I renamed it in the following, to avoid confusion:
b53f3ce

@Ivansete-status

Ivansete-status commented May 12, 2026

Copy link
Copy Markdown
Collaborator Author

Yepp, this is needed but I see a bit of discrepancy ...

Wow! Great catch, thanks @NagyZoltanPeter !
I didn't properly solve the conflicts after rebasing and it got messy. I think it is better now. Addressed in dbc5852

@Ivansete-status
Ivansete-status merged commit 6d31fa3 into master May 12, 2026
12 checks passed
Ivansete-status added a commit that referenced this pull request Jun 4, 2026
CBOR is the headline 0.2.0 feature, not 0.1.4: at v0.1.4 serial.nim was
still JSON/string-based, so the prior CBOR attribution was wrong. Also
complete the 0.2.0 scope (events, registry, codegen) ahead of tagging.

Date 0.1.4 by its last functional change (#14, 2026-05-13) rather than
the later changelog/version-bump commits, so the version reflects when
its code actually settled.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
gmelodie pushed a commit that referenced this pull request Jun 13, 2026
CBOR is the headline 0.2.0 feature, not 0.1.4: at v0.1.4 serial.nim was
still JSON/string-based, so the prior CBOR attribution was wrong. Also
complete the 0.2.0 scope (events, registry, codegen) ahead of tagging.

Date 0.1.4 by its last functional change (#14, 2026-05-13) rather than
the later changelog/version-bump commits, so the version reflects when
its code actually settled.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

leaked file descriptors

3 participants