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

V4 leak fix #288

Merged
merged 12 commits into from
Apr 14, 2021
Merged

V4 leak fix #288

merged 12 commits into from
Apr 14, 2021

Conversation

Awfa
Copy link

@Awfa Awfa commented Mar 18, 2021

Fixes #287

Copy link
Member

@0xpr03 0xpr03 left a comment

Choose a reason for hiding this comment

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

Thanks for the work!

src/windows.rs Show resolved Hide resolved
@0xpr03 0xpr03 requested a review from JohnTitor March 18, 2021 19:45
@0xpr03
Copy link
Member

0xpr03 commented Mar 18, 2021

Note: I'll do a v4 release once this is merged.

Copy link
Member

@JohnTitor JohnTitor left a comment

Choose a reason for hiding this comment

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

Could we add some tests somehow? I don't think v4 is actively developed but I'd like to avoid the regression by a further patch.

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Yuki Okushi <[email protected]>
@Awfa
Copy link
Author

Awfa commented Mar 25, 2021

Could we add some tests somehow? I don't think v4 is actively developed but I'd like to avoid the regression by a further patch.

Hmm - I don't know how we can trigger the specific race using the public interface of ReadDirectoryChangesWatcher - which the test cases use.

@Awfa
Copy link
Author

Awfa commented Apr 2, 2021

@0xpr03 do you know if those test failures are from just flaky tests? I remember the previous test run didn't have those failures.

@0xpr03
Copy link
Member

0xpr03 commented Apr 2, 2021

You can ignore macos tests.

@JohnTitor
Copy link
Member

Hmm - I don't know how we can trigger the specific race using the public interface of ReadDirectoryChangesWatcher - which the test cases use.

Hm fair enough, anyway the fix is obvious so I'm fine as-is.

do you know if those test failures are from just flaky tests?

Yeah, currently GHA doesn't support the allow-failure feature.

@Awfa
Copy link
Author

Awfa commented Apr 4, 2021

Alright - what's the next step to get this PR in? I'm not sure how to rerun the checks.

@JohnTitor
Copy link
Member

@0xpr03 Feel free to merge this (and make a new release if needed) if you think we don't need further action here.

@0xpr03
Copy link
Member

0xpr03 commented Apr 4, 2021

Yeah I'll have to rebase this later to merge it, according to github. And will do a new release, we've gathered some stuff for the v4. Also I'll make a PR for the 3 liner to v5 and cc @Awfa to make sure it's the correct one.

@0xpr03 0xpr03 merged commit 54079d9 into notify-rs:try-v4 Apr 14, 2021
0xpr03 added a commit that referenced this pull request Apr 14, 2021
0xpr03 added a commit that referenced this pull request Apr 14, 2021
0xpr03 added a commit that referenced this pull request Apr 15, 2021
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.

3 participants