Deactivate thread while suspended by oleavr · Pull Request #2282 · svaarala/duktape · GitHub
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

Deactivate thread while suspended #2282

Merged
merged 1 commit into from
Apr 26, 2020

Conversation

oleavr
Copy link
Contributor

@oleavr oleavr commented Apr 26, 2020

Even though another native thread runs code on a different Duktape
thread, its code may trigger finalizers, and those always run on the
heap thread. And the heap thread may have been the one that was
suspended. This would in turn result in duk__call_thread_state_update()
failing, and finalizers being skipped, typically resulting in memory
leaks.

Even though another native thread runs code on a different Duktape
thread, its code may trigger finalizers, and those always run on the
heap thread. And the heap thread may have been the one that was
suspended. This would in turn result in duk__call_thread_state_update()
failing, and finalizers being skipped, typically resulting in memory
leaks.
@svaarala
Copy link
Owner

Thanks!

Do you have a repro case?

If not, I can try to come up with one to cover this in the API tests. I guess one would just create a bunch of finalizable objects, suspend the main thread, make the objects unreachable and force a GC. Then observe that the finalizers are actually called to verify this issue didn't happen.

@svaarala svaarala added this to the v3.0.0 milestone Apr 26, 2020
@svaarala svaarala merged commit 19f7983 into svaarala:master Apr 26, 2020
@svaarala svaarala mentioned this pull request Apr 26, 2020
21 tasks
svaarala added a commit that referenced this pull request Apr 26, 2020
@svaarala
Copy link
Owner

Okay, managed to create a repro => #2285.

Before fix:

*** test_basic (duk_safe_call)
other thread executing
forcing gc
forced gc done, now resume
main thread back
final top: 0
==> rc=0, result='undefined'

After fix:

+*** test_basic (duk_safe_call)
+other thread executing
+finalizer 2 called!
+forcing gc
+finalizer 1 called!
+forced gc done, now resume
+main thread back
+final top: 0
+==> rc=0, result='undefined'

svaarala added a commit that referenced this pull request Apr 26, 2020
svaarala added a commit that referenced this pull request Apr 26, 2020
@oleavr
Copy link
Contributor Author

oleavr commented Apr 27, 2020

Yay, thanks a lot! (Didn't have a repro case isolated to Duktape, so that's awesome! 🤘)

@oleavr oleavr deleted the fix/thread-suspend-state branch April 27, 2020 16:51
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.

2 participants