Introduce an alternative macOS implementation that uses kqueue…#4
Draft
savetheclocktower wants to merge 2 commits intomasterfrom
Draft
Introduce an alternative macOS implementation that uses kqueue…#4savetheclocktower wants to merge 2 commits intomasterfrom
kqueue…#4savetheclocktower wants to merge 2 commits intomasterfrom
Conversation
…to better match this library's pre-v9 behavior.
Author
|
…OK. Yeah, no regressions on the other platforms. I will spin up a draft PR for Pulsar and see if this improves things. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…to better match this library's pre-v9 behavior.
pulsar#1459 is vexing because I've not yet found a good way to reproduce the conditions in which it manifests. The outward symptom is that Pulsar fails to recognize when one of the open buffers has its contents changed on disk by another program.
So far, this is only affecting macOS. That tracks; even though this library was nearly entirely rewritten for version 9, Windows and Linux still use the exact same mechanisms for file-watching as they did before (
ReadDirectoryChangesWandinotify, respectively). But on macOS, we moved fromkqueuetoFSEventson the theory that the latter is newer and theoretically better for file-watching. (It supports recursive file-watching, for instance; while this isn't something thatpathwatcherspecifically needs, it opens up opportunities for streamlining of watchers.)But it's different — which means that it will behave differently in some edge cases! And I'm starting to think that
pathwatcherwas right to stick withkqueuefor its use cases.There are a few reasons that this could happen:
A stream could fail to start. Since a single stream can watch an arbitrary number of paths, we need only two: the one that's currently running, plus a backup stream. When we add a new path to the list of paths to watch, we start up the backup stream, promote it to the primary stream, then stop the other stream and demote it to backup status. This staggered approach is designed to ensure we don't miss anything.
But the stream could fail to start! The API docs say this “isn't supposed to happen,” but that API consumers should check the return value and fall back to another approach. In practice, I've seen this happen when
fseventsdis maxed-out on the number of streams (I think it's 1024).A stream could successfully start… but still somehow miss a filesystem event. I don't know under what circumstances this could happen… but it's definitely happening, and it's hard to debug. For instance, this comment suggests that iCloud Drive sharing permissions may factor into it; Claude theorizes that a read-only file might actually be stored elsewhere on the filesystem, and that
fseventsdmight fail to detect edits made by others.Without a good ability to diagnose this, I decided to introduce a
kqueueimplementation of file-watching on macOS that could be swapped in. The main implementation difference inkqueueis that you must subscribe to a specific file to detect changes to the file; withFSEvents(and other mechanisms) you can listen on the parent directory to pick up those changes. In practice, this doesn't make much of a difference, though.It was surprisingly easy to make these tests pass with the
kqueueimplementation. I don't think any of the changes I had to make in the JS affect the other platforms, but I suppose I'll find out when I look at the CI job results.Anyway: I can't promise that this will fix all file-watching bugs. But
kqueueis probably better for this use case: it's a lower-level API built into the kernel, it fires events with less latency, and it's much easier to find out why the watcher has triggered. And though I didn't rely on any of the previouspathwatchercode when adding thiskqueueimplementation, I think it's much more likely to behave like the v8-and-belowpathwatcher.There is a theoretical limit to how many files can be monitored this way by
kqueue— but the limit is per-process. Since each window is its own renderer process, this gives us a lot of flexibility. And though the default limit is 256, that's actually fine in most scenarios — and we can increase it if need be viasetrlimit.If the specs pass, I think we should give this a try in the next regular release, or at least open a draft PR so that those affected by pulsar#1459 can try it out for a while and tell us if it helps. I wouldn't say I'm ready to abandon the
FSEventsadapter altogether, so it's worth keeping around while we're experimenting. (Originally I wanted the ability to opt into one implementation or another at runtime, but that would've ramped up the complexity of this PR, and I really just wanted to get something out there.)