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

Use PollWatcher in memofs StdBackend on macOS #783

Merged
merged 5 commits into from
Sep 26, 2023

Conversation

kennethloeffler
Copy link
Member

@kennethloeffler kennethloeffler commented Sep 17, 2023

This should hopefully finally put to rest the extreme slowness on macOS. Taken from UpliftGames#2.

Looks like it fails end to end serve tests because write events are never received...☹Interestingly, this is similar to something that @sasial-dev observed a little while ago in the OSS Discord server, I wonder if it's related? It might tank this whole PR, unfortunately

@kennethloeffler kennethloeffler marked this pull request as draft September 17, 2023 12:02
@kennethloeffler
Copy link
Member Author

Ok, looks like the issue is confined to serve tests only (it works as expected during real usage). I'll work on sussing out the cause

@kennethloeffler
Copy link
Member Author

kennethloeffler commented Sep 23, 2023

Would it be ok to just put this behind a #[cfg(target_os = "macos")], and throw some macOS testers at it once we cut the 7.4.0 prerelease? Having a lot of trouble root causing the test failure, I can't even reproduce it outside of the serve tests

@kennethloeffler
Copy link
Member Author

OK, so I got @sasial-dev to test this and serve tests pass using PollWatcher on macOS [???]

I'll gladly take this, but it's pretty spooky so we should investigate further

@kennethloeffler kennethloeffler changed the title Use PollWatcher in memofs StdBackend Use PollWatcher in memofs StdBackend on macOS Sep 25, 2023
@Dekkonot
Copy link
Member

I am uncomfortable with this set of problems because it feels suspiciously like a race condition.

You can read the core loop of PollWatcher here. It's very simple, and there's only a few places that platform specific behavior could be in play. These are, by my count: std::fs, walkdir, and filetime. I'm inclined to believe these don't have critical flaws in them such that they are causing our problems; filetime does platform-specific syscalls but it's relied upon by Cargo, so I would hope that someone would have noticed any serious bugs by now.

My suspicion is that we're deadlocking somewhere because Github CI runners only have 2 cores. This limits the amount of threads we can run quite severely. On local machines, this limit is bypassed even if you limit the number of jobs cargo test can use because the serve tests spawn a child process that won't respect it. I don't have a dual core processor on hand to test with (who does?) but I suspect we'd run into the same issue regardless of OS.

Either way, I don't think this is a problem we need to worry about. I'm comfortable enough with my audit that I'm willing to approve this since I don't believe it's some deep seated systemic bug in Notify.

@kennethloeffler kennethloeffler marked this pull request as ready for review September 25, 2023 23:31
@Dekkonot Dekkonot merged commit 3cafbf7 into rojo-rbx:master Sep 26, 2023
4 checks passed
@kennethloeffler kennethloeffler deleted the memofs-use-poll-watcher branch September 26, 2023 22:42
@Dekkonot Dekkonot mentioned this pull request Nov 9, 2023
kennethloeffler added a commit to kennethloeffler/rojo that referenced this pull request Jan 2, 2024
kennethloeffler added a commit to kennethloeffler/rojo that referenced this pull request Jan 2, 2024
Dekkonot pushed a commit to UpliftGames/rojo that referenced this pull request Jan 11, 2024
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.

2 participants