-
Notifications
You must be signed in to change notification settings - Fork 9
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
fix: thorvg engine init and termination with reference counting of renderers #265
Conversation
aac6e34
to
52829ff
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left just two comments because it seems to me like the approach won't work without some locking mechanism.
af9d870
to
faaf468
Compare
04a9892
to
196e655
Compare
196e655
to
09b4410
Compare
2ec3604
to
e7810ef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with 2 comments that you might want to take a look at.
use std::thread; | ||
|
||
#[test] | ||
fn test_tvg_renderer_no_deadlock() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a nit: this test might not actually capture a deadlock, so you can have false negatives here. It's still useful as a smoke test, but it might lead to a case like this:
- Alice make a PR with a concurrency bug, but the test happens to pass and is merged.
- Bob makes another PR that has nothing to do with concurrency, but his PR fails this regression test.
Up to you what you wanna do with it. :)
e226d0c
to
aeab801
Compare
This update ensures that
tvg_engine_init
is called only once, provided no renderers are already created. It also ensures thattvg_engine_term
is called when the last renderer is dropped, resolving an issue where creating a new renderer would unnecessarily reinitialize the engine and dropping it would attempt to terminate it again