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 deadlock in debouncer #210

Merged
merged 3 commits into from
Oct 16, 2019
Merged

Conversation

mtak-
Copy link

@mtak- mtak- commented Sep 11, 2019

Works around the issue described in #205. Tested on the fsevent backend only linux/macos/windows.

I tried to change locking as little as possible as it's somewhat hard to follow, and instead use a simple backoff algorithm to workaround the deadlock.

These tests fail on both branch origin/try-v4 and my fork.

test create_rename_overwrite_directory ... FAILED
test create_rename_overwrite_file ... FAILED

Would be nice to get a new version out with the fix as well!

@mtak- mtak- force-pushed the fix-macos-debounce-deadlock branch from a9d4fec to b2792e1 Compare September 11, 2019 23:10
@mtak- mtak- force-pushed the fix-macos-debounce-deadlock branch from b2792e1 to 977ecd0 Compare September 11, 2019 23:29
@mtak-
Copy link
Author

mtak- commented Sep 11, 2019

Many of the ci failures existed previously: https://travis-ci.org/passcod/notify/builds/579460084

@mtak-
Copy link
Author

mtak- commented Sep 12, 2019

All of the failures are preexisting, the windows stable failure that appears new I'm guessing is a flaky test truncate_write_file? On the previous build, nightly failed with the same error.

@Dylan-DPC

@0xpr03
Copy link
Member

0xpr03 commented Sep 17, 2019

I'll have to check this when I've got some time. Please ping me again otherwise in 3 days.

@0xpr03
Copy link
Member

0xpr03 commented Sep 17, 2019

I think we want to warn users about the broken state of the debouncer in these cases, if we're not able to fix it sufficiently. This affects at least 4.X stable and 5.X. So breaking the debouncer isn't possible. Your PR also mentions macOS specifically. This issue persists at least also on linux, it may also be another issue with similar symptoms.
cc @Dylan-DPC

@mtak-
Copy link
Author

mtak- commented Sep 25, 2019

The delay_zero test repros the issue pretty consistently on macos. If someone wants to test linux, just pasting that test into the current version should hopefully fail. Whereas running that test against this PR should hopefully succeed.

- panic on deadlock in delay_zero test.
- don't print spammy messages to terminal
@mtak- mtak- force-pushed the fix-macos-debounce-deadlock branch from 737b5d9 to 0b632d7 Compare September 25, 2019 19:25
@mtak-
Copy link
Author

mtak- commented Sep 25, 2019

I pushed up the delay_zero test without the deadlock fix workaround, to let travis test the platforms I don't have access to (windows/linux). The test failed on both platforms here. Altho the delay_zero test did pass one of the linux builds (it is a non-deterministic test).

Testing both platforms again with the deadlock fix shows the delay_zero test passing on all platforms here.

@mtak- mtak- changed the title fix deadlock in macos backend fix deadlock in debouncer Sep 25, 2019
@0xpr03
Copy link
Member

0xpr03 commented Sep 26, 2019

Thanks for all the work. I re-scheduled CI and will verify on linux again.

@0xpr03
Copy link
Member

0xpr03 commented Oct 16, 2019

I can confirm that this resolves the deadlock problem under linux as well.
I can't say why truncate_write_file failed on windows for 1.26 but would merge this. cc @JohnTitor

@0xpr03
Copy link
Member

0xpr03 commented Oct 16, 2019

truncate_write_file is broken already so I'd go ahead and merge this

@0xpr03 0xpr03 merged commit 6ccf3e8 into notify-rs:try-v4 Oct 16, 2019
@JohnTitor
Copy link
Member

JohnTitor commented Oct 17, 2019

@0xpr03 Windows failure is caused by installation windows-sdk. b66d724 fixes this problem so you can cherry-pick this commit to try-v4 branch.

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