PetterS / quickjs

Thin Python wrapper of https://bellard.org/quickjs/
https://github.com/bellard/QuickJS
MIT License
177 stars 19 forks source link

Memory leaks with callables #77

Closed qwenger closed 2 years ago

qwenger commented 2 years ago

The tests test_can_eval_in_same_context and test_can_call_in_same_context leak memory according to check_memory.py.

MWE:

import _quickjs
context = _quickjs.Context()
context.add_callable("p", context.get("parseInt"))

In this example, context_dealloc won't ever be called, as can be shown using gdb and setting a breakpoint on it (the breakpoint won't trigger). Executing context.gc() or del context or gc.collect() do not help. Nor does deleting p in JS with a context.eval.

The reason may revolve around wrapped JS objects that retain a reference to the context, though I don't see a clear culprit yet.

PetterS commented 2 years ago

Thanks for pointing this out. We should fix this.

821938089 commented 2 years ago

add_callable bind an instance method can also cause memory leaks

example script

import quickjs

class JsRuntime:
    def __init__(self):
        self.context = quickjs.Context()
        self.context.add_callable('some_func', self.some_func)

    def some_func(self):
        pass

def main():

    for _ in range(10000):
        JsRuntime()

    input('ok')

if __name__ == '__main__':
    main()

run the script and open task manager image

qwenger commented 2 years ago

@821938089 Could you please check your code against the PR #78 ? It may fix your issue as well.

Also, does it help to run the GC explicitly (import gc, gc.collect()), either in or after the loop?

EDIT: just tested. 1) Adding explicit GC runs doesn't help, 2) Using the code from #78 does fix it for me.

821938089 commented 2 years ago

It also fix for me.

qwenger commented 2 years ago

Nice! Then we need to wait for @PetterS to take action on #78 .