Skip to content

Additional cleanup in oni_destroy_ctx#58

Merged
jonnew merged 2 commits intomainfrom
fix-memory-leak
Mar 31, 2026
Merged

Additional cleanup in oni_destroy_ctx#58
jonnew merged 2 commits intomainfrom
fix-memory-leak

Conversation

@jsiegle
Copy link
Copy Markdown
Member

@jsiegle jsiegle commented Feb 25, 2026

I needed to add these additional lines to avoid memory leaks on Windows with the acquisition board plugin. Let me know if this makes sense to merge in main.

@bparks13 bparks13 requested review from aacuevas and jonnew February 25, 2026 23:21
Copy link
Copy Markdown
Member

@jonnew jonnew left a comment

Choose a reason for hiding this comment

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

Although this commit generally speaks to architectural issues with the calling code, I think that there are some points in here worth thinking about. This library is not thread safe and I have suspicion that the issue here might be resulting from the calling code using different threads for context management a reading frames.

  1. Replacing the actions proposed in this comment with debug assertions about non-zero ref counts at time of oni_destroy_ctx call. This seems obvious for guiding devleopment.

  2. Freeing the dev_table does seem like a valid addition.

Comment thread api/liboni/oni.c
Comment thread api/liboni/oni.c
@jonnew
Copy link
Copy Markdown
Member

jonnew commented Mar 17, 2026

@aacuevas I would like your opinion here as well.

@jonnew jonnew self-requested a review March 20, 2026 15:19
Copy link
Copy Markdown
Member

@jonnew jonnew left a comment

Choose a reason for hiding this comment

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

After the discussion with @aacuevas I realized my inital review as wrong. My only suggestion is to add an NB:

    // NB: _ref_dec is only called when a new shared buffer is created. We must
    // therefore decrement the active shared buffers here to balance the initial
    // _ref_inc from their creation.
    if (ctx->shared_rbuf != NULL)
        _ref_dec(&(ctx->shared_rbuf->count));

since this is a bit tricky.

- Added NB to ref count decrement action in oni_destory_ctx to explain
  reasoning
@jonnew jonnew merged commit 4235030 into main Mar 31, 2026
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.

3 participants