Chakracore: Memory leak in JsSetCurrentContext

[benjamin-bader]

Hi folks,

I've noticed that JsSetCurrentContext seems to leak small amounts of memory; see the screenshot below for a backtrace from Instruments on macOS. We're using ChakraCore in a server process, and during stress testing yesterday I noticed that we were leaking memory. Instruments shows that JsSetCurrentContext calls appear to be the cause; all roads lead to ThreadContextTLSEntry in the backtraces.

Is it possible that there's some required cleanup that I'm missing here? In our current design, request threads "borrow" a runtime from a pool, so we end up calling JsSetCurrentContext at least once per request. Are we supposed to "un-set" the context when a thread is finished using a runtime?

[benjamin-bader]

FYI, I did a quick spike of a different architecture, where each runtime is confined to a single thread. I didn't change any code that calls ChakraCore functions, just which threads do the calling.

The leak disappeared - makes me think that TLS entries are not freed when their associated thread dies. I don't know if this is by-design, but this approach seems to fix my immediate problem. Thanks again!

[jackhorton]

I believe all access to ChakraCore runtimes is supposed to happen on the same thread on which it was created (literally "the script thread" or, in browsers, "the UI thread"), so I am actually surprised this is the only issue that you ran into by calling Js* functions from thread A for contexts/runtimes in thread B

[benjamin-bader]

Interesting! Somehow, I missed that point; thanks for making it clear.

Regardless, I've worked around my problem; if you all consider this to be working as designed, please feel free to close the issue.

[jackhorton]

Ill wait until someone with more knowledge on the JsRT chimes in before closing it to make sure.

[liminzhu]

Nah you can definitely move runtime around and use it from different threads. The threading model is called rental threading.

[jackhorton]

I see. So then this leak sounds like a bug on our end?

[curtisman]

On windows, the TLS entry will be deleted when the thread exits. That is by design.

[jackhorton]

Looks like this is a "known" issue: https://github.com/Microsoft/ChakraCore/blob/5f4fe230acc4c8e99dee252382b2b20ae1f0a0fd/bin/ch/ch.cpp#L813

[dilijev]

@leirocks @curtisman Since this is by design, should we close this issue?

[curtisman]

Seems to be a xplat only issue.

[jackhorton]

Well, by design may be a bit aggressive, since the design seems to be "we don't know how to do this on xplat right now". If there is a comment in the code base acknowledging that there is an issue, I think there should be an actual open issue for it.

[benjamin-bader]

I agree with @jackhorton - this definitely seems to be a known deficiency, rather than intended behavior. If you all decide that this isn't worth fixing, "wontfix" would be more appropriate than "By Design".

...I'd still vote for fixing, though 🙏 🙏 🙏