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

fix(base-driver): Update/simplify the logic for logger prefix #16683

Merged
merged 10 commits into from
Apr 1, 2022
Merged

fix(base-driver): Update/simplify the logic for logger prefix #16683

merged 10 commits into from
Apr 1, 2022

Conversation

mykola-mokhnach
Copy link
Collaborator

Proposed changes

This restores #16636 with some slight changes to make ts linter happy

@boneskull
Copy link
Contributor

Can you explain what this avoids?

@mykola-mokhnach
Copy link
Collaborator Author

This avoid possible memory leak in case the logger object itself is preserved somewhere. Without WeakRef the actual driver instance would be preserved with it as it is captured in the corresponding closure.

Copy link
Contributor

@boneskull boneskull left a comment

Choose a reason for hiding this comment

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

I don't think we need WeakRef here... please see comment.

packages/base-driver/lib/basedriver/core.js Outdated Show resolved Hide resolved
@mykola-mokhnach
Copy link
Collaborator Author

mykola-mokhnach commented Mar 29, 2022

I have updated the implementation, but now I have no idea about how to update unit tests after _log object gets reassigned in createSession. I also don't have a good idea on how to make prefix mutable on the logger itself

@boneskull
Copy link
Contributor

@mykola-mokhnach Can I push to your branch?

@mykola-mokhnach
Copy link
Collaborator Author

@mykola-mokhnach Can I push to your branch?

sure, not a problem at all

@mykola-mokhnach
Copy link
Collaborator Author

Seems like I got some progress as of now. Please review once more @boneskull

Copy link
Contributor

@boneskull boneskull left a comment

Choose a reason for hiding this comment

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

This is good and was basically what I was suggesting, so thanks.

For a test I think all you really need to do is have something that compares this.log.prefix before and after calling createSession(), and again for deleteSession(). We can assert the prefix looks a certain way, or we can just assert it's not the same.

You'd probably want to put that in the baseDriverUnitTests function of test/basedriver/driver-tests.js

packages/base-driver/lib/basedriver/core.js Outdated Show resolved Hide resolved
packages/types/src/index.ts Show resolved Hide resolved
packages/types/src/index.ts Outdated Show resolved Hide resolved
packages/types/src/index.ts Outdated Show resolved Hide resolved
@mykola-mokhnach mykola-mokhnach changed the title fix(base-driver): Restore WeakRef logic which was accidentially removed fix(base-driver): Update/simplify the logic for logger prefix Apr 1, 2022
@mykola-mokhnach mykola-mokhnach merged commit a9651d3 into appium:2.0 Apr 1, 2022
@mykola-mokhnach mykola-mokhnach deleted the weak_ref branch April 1, 2022 20:34
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.

None yet

3 participants