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.

Sign up for GitHub

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

Jump to bottom

Fix tests for memory leak patch #15

Closed
wants to merge 6 commits into from
Closed

Fix tests for memory leak patch #15

wants to merge 6 commits into from

Conversation

mattseddon
Copy link

@mattseddon mattseddon commented Nov 23, 2021 •
edited
Loading

@rickycao-qy I have been looking into #10 and saw that you had a fix for it prepared in #12.

I've spent some time getting the tests to pass locally. Would you be able to take a look for me?

Thanks,
Matt

mattseddon
mattseddon commented Nov 23, 2021
View reviewed changes
src/core/reference.cc Outdated Show resolved Hide resolved
@mattseddon mattseddon marked this pull request as ready for review November 23, 2021 02:41
@mattseddon
Copy link
Author

Relates to iterative/vscode-dvc#967

@mattseddon
Copy link
Author

@FeelyChau would you be able to confirm if this fixes the issue?

@mattseddon mattseddon changed the base branch from fix/memory-leak to main December 12, 2021 02:16
@mattseddon mattseddon mentioned this pull request Dec 21, 2021
Open
@FeelyChau
Copy link
Contributor

@FeelyChau would you be able to confirm if this fixes the issue?

Thanks for the contribution and sorry for being late.
Need to fix the lint error first.

@mattseddon
Copy link
Author

@FeelyChau would you be able to confirm if this fixes the issue?

Thanks for the contribution and sorry for being late. Need to fix the lint error first.

Sorry, that check skipped locally for me because I didn't have clang-format installed:

/boa fix/memory-leak-tests *1 ❯ npm run lint

> @pipcook/[email protected] lint
> npm run lint:js && npm run lint:clang


> @pipcook/[email protected] lint:js
> eslint . -c .eslintrc.js --no-eslintrc


> @pipcook/[email protected] lint:clang
> ./clang-format.py

No clang-format-9.0 found, skipping checks!
* total errors: 0

Should be fixed now.

mattseddon
mattseddon commented Dec 22, 2021
View reviewed changes
src/core/reference.cc
@@ -6,12 +6,6 @@ PythonReference::PythonReference(PythonObject *scope) : mScope(scope) {
mCapsule = PyCapsule_New(this, nullptr, nullptr);
}

PythonReference::~PythonReference() {
Copy link
Author

@mattseddon mattseddon Dec 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[F] This Unref was causing the same errors shown in nodejs/node-addon-api#998. Removing the Unref gets rid of the errors. The explanation for why we shouldn't be doing this is shown in the issue here.

I originally left the method in and empty but that led to linting errors. We need this method or the tests fail 🤦🏻 😢 .

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😢

@mattseddon mattseddon force-pushed the fix/memory-leak-tests branch 2 times, most recently from dc4f821 to 307459d Compare December 23, 2021 21:26
mattseddon
mattseddon commented Dec 23, 2021
View reviewed changes
tests/base/async.js
@@ -47,7 +47,6 @@ if (isMainThread) {
console.log('train task is completed');
setTimeout(() => {
clearInterval(alive);
console.log(foo.ping('x'));
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[F] This call is to an object that is in the middle of garbage collection. I could get the call to work by moving it outside of setTimeout on node v14 but not v12. I would assume that is because garbage collection works slightly differently between the two versions. I would also assume that the test worked previously because the object was not being collected at this point. Please let me know if you disagree with these assumptions.

Thanks

yorkie
yorkie requested changes Dec 24, 2021
View reviewed changes
package.json Show resolved Hide resolved
src/core/object.cc Outdated
info.Env(), pybind::reinterpret_borrow<pybind::object>(result));
Py_DECREF(result);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pybind::reinterpret_borrow holds the memory of result, this call is unnecessary.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was @rickycao-qy's change, I will remove.

yorkie
yorkie approved these changes Dec 29, 2021
View reviewed changes
Copy link
Member

@yorkie yorkie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thank you @mattseddon

@rickycao-qy rickycao-qy mentioned this pull request Dec 29, 2021
Closed
@mattseddon
Copy link
Author

@FeelyChau can this get merged now?

@mattseddon
Copy link
Author

Any plans to merge? Please lmk and I will close.

@mattseddon mattseddon closed this Jun 9, 2022
@mattseddon mattseddon deleted the fix/memory-leak-tests branch June 9, 2022 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yorkie yorkie yorkie approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mattseddon @FeelyChau @yorkie @rickycao-qy