HiRoFa / quickjs_es_runtime

this is a wrapper library for the javascript runtime quickjs written in rust which works with typescript, modules, promises, async, await and much more
https://github.com/HiRoFa/quickjs_es_runtime
MIT License
104 stars 12 forks source link

Segmentation fault: 11 & panic when using ScriptModuleLoader<QuickJsRealmAdapter> #65

Closed SreeniIO closed 2 years ago

SreeniIO commented 2 years ago

I've created a minimal reproducible repo

Running multi_context causes SIG 11 and running single_context panics.

#[tokio::main]
async fn main() -> anyhow::Result<()> {
    let rt = make_rt();

    multi_context(rt).await?;
    // Segmentation fault: 11 [after 4520]

    // single_context(rt).await?;
    // thread '<unnamed>' panicked at 'could not create func', .../quickjs_runtime-0.8.0/src/esvalue.rs:365:6
    // [after 7215]

    println!("done");

    Ok(())
}

Everything works find when executing js without any module imports. i.e. by commenting the below lines from the js the loop completes without any error.

                            // const { abc } = await import('test');
                            // abc();

Note: In our actual code, we don't have this loop, but still getting these errors after few minutes/hours of running the server depending on the load.

andrieshiemstra commented 2 years ago

Hmm, i managed to reproduce it with your repo, for me it crashes at 2438.

At first glance you seem to be awaiting your asyncs correctly allthough the problem does seem to be an async promise being resolved after the context was destroyed

I'm not completely sure about the soundness of EsValueFacade with these things so the first thing i'll do is see if the same problem can be reproduced using JsValuesFacade..

I'll get back to you

andrieshiemstra commented 2 years ago

Disabling the memory_limit

pub fn make_rt() -> Arc<QuickJsRuntimeFacade> {
    let rt = Arc::new(
        QuickJsRuntimeBuilder::new()
            //.memory_limit(1024 * 1024 * 6400)
            .script_module_loader(Box::new(ModuleLoader::new()))
            .build(),
    );
    rt.set_function(vec!["xconsole"], "log", js_debug)
        .ok()
        .expect("set_function failed");
    rt
}

Seems to do wonders....

First of all, it's something i need to handle gracefully (e.g. the .expect("cant create func") is evil because i really need to expect objects not beeing able to be created because of memory limits

second.. you set the limit to 6.4GB, your test app used like no memory at all so apparently the limit does not work as expected

SreeniIO commented 2 years ago

Not specifying the memory_limit seems to fix the issue. But, the memory usage keeps increasing on every import call.

Based on the below screenshot, about 130+ MB memory has been consumed and not released in just 4 minutes.

image

andrieshiemstra commented 2 years ago

Hi, i tested with valgrind and time and cannot find memory leaks with your test code

Allthough it isn't impossible there is a mem leak in my lib i don't see it right now

cargo build --release
valgrind --leak-check=full --show-leak-kinds=all --error-exitcode=1 ./target/release/jstest
cargo build --release
/usr/bin/time -v target/release/jstest

with 2.000 iterations time shows

Maximum resident set size (kbytes): 5196

and with 20.000 iterations time show

Maximum resident set size (kbytes): 5116

If you do come up with a test lib which shows memory not being released i'd be glad to fix it, i'll keep an eye on this behaiviour myself also

SreeniIO commented 2 years ago

Will try to update the minimal repo and will get back to you if I can reproduce it there.

SreeniIO commented 2 years ago

Will try to update the minimal repo and will get back to you if I can reproduce it there.

The memory leak was in our custom TypeScriptPreProcessor. So, please ignore the above memory leak issue.

andrieshiemstra commented 2 years ago

Will try to update the minimal repo and will get back to you if I can reproduce it there.

The memory leak was in our custom TypeScriptPreProcessor. So, please ignore the above memory leak issue.

Glad you found it!, I'll work om getting mem limit to work in another ticket.