-
Notifications
You must be signed in to change notification settings - Fork 600
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
nodejs: Add a test for callback garbage collection #3864
base: master
Are you sure you want to change the base?
Conversation
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.
So there was no leak at all after all?
Yes, at least I couldn't find it. But what's weird is that I don't quite understand how / why it works. But if the test looks sane to you, too, then that's the best thing we have for now to verify that we're not leaking. |
Is there a test that doesn't do |
api/node/__test__/api.spec.ts
Outdated
demo = null; | ||
callback_invoked = true; | ||
}; | ||
|
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.
What if you add something like this:
function scope() { | |
let copy = demo; | |
copy.say_hello = () => { | |
console.log(copy.check); | |
}; | |
} | |
scope(); |
i.e: add another callback that use a property from some other variable with a small scope.
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.
That did the trick, good idea. Pushed, now the test fails.
api/node/__test__/api.spec.ts
Outdated
let demo = loadFile(path.join(__dirname, "resources/test-constructor.slint")) as any; | ||
let callback_invoked = false; |
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.
Stupid bug here: The instance is never created! The my_callback
assignments happen to work because well, it's just a JS object. And because we don't have type safety, the compiler didn't warn us. So the test never really tested what it should have: instantiate the Rust code that presumably keeps a strong ref.
Note to future self:
|
This should never have happened. I'll add a fix to seal our instance and test that.
d9ce2ae
to
b551105
Compare
Rebasing this first and then trying to find out why it passes in the CI (but fails, as expected locally). |
Replaces #3772
Closes #3706