Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can quickjs be used in multithreading? #362

Open
penneryu opened this issue Oct 15, 2024 · 3 comments
Open

Can quickjs be used in multithreading? #362

penneryu opened this issue Oct 15, 2024 · 3 comments

Comments

@penneryu
Copy link

penneryu commented Oct 15, 2024

static JSClassID js_class_id_alloc = JS_CLASS_INIT_COUNT;

/* a new class ID is allocated if *pclass_id != 0 */
JSClassID JS_NewClassID(JSClassID *pclass_id)
{
    JSClassID class_id;
#ifdef CONFIG_ATOMICS
    pthread_mutex_lock(&js_class_id_mutex);
#endif
    class_id = *pclass_id;
    if (class_id == 0) {
        class_id = js_class_id_alloc++;
        *pclass_id = class_id;
    }
#ifdef CONFIG_ATOMICS
    pthread_mutex_unlock(&js_class_id_mutex);
#endif
    return class_id;
}

As shown in the code above, js_class_id_alloc is a global variable, and the same reference is used in all threads.
If different JSRuntimes are instantiated in different threads, and JS_NewClassID is called in each thread to generate a new classId, then the classId will not grow independently in each thread as expected, which should cause management problems for class_array in each JSRuntime.

ps: In addition, I found that calling quickjs API in multiple threads will cause some resource management problems, even in different JSRuntime instances.

@bnoordhuis
Copy link
Contributor

I don't know if Charlie and Fabrice intend to cherry-pick it but we fixed that in quickjs-ng almost a year ago to the day in commit quickjs-ng/quickjs@5ce2957e.

@penneryu
Copy link
Author

I don't know if Charlie and Fabrice intend to cherry-pick it but we fixed that in quickjs-ng almost a year ago to the day in commit quickjs-ng/quickjs@5ce2957e.

thanks, I think this modification is good, classid is originally bound to the runtime, so creating a new one should not affect other runtimes.

@chqrlie
Copy link
Collaborator

chqrlie commented Nov 16, 2024

Yes, we shall cherry-pick this patch and many others

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

No branches or pull requests

3 participants