-
Notifications
You must be signed in to change notification settings - Fork 246
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
Recursion is used for OS X adapters when it need not be #307
Comments
Recursion is a limitation of rb-fsevent. There might be a way to avoid it, but that requires OSX development, which I can't do (I'm Linux-only). Overall, rb-fsevent is capable of monitoring changes to files, but this was never enabled because AFAIR watching files (see rb-fsevent docs) generated too many events. So ... the Darwin adapter in Listen is basically "half-polling" - when something changes in a directory, that directory is scanned. You're right - there's no reason other adapters need it (except polling). I wouldn't mind accepting patches that add adapter-specific options (open an issue + ask for help if you want to take a stab at it). The best "workaround" is probably to ignore directories you don't care about (so they won't be scanned). If you think OSX adapter is scanning more than it should - you may be right. The It's possible to speed up the scanning a lot - mostly by avoiding Celluloid and avoiding using Pathnames where strings could be used (Pathname is quite slow). I've wanted to get to this for a while - but it might take time to make it stable enough to release. One final note: it's best to be specific when talking about the use case, because there are so many caveats. E.g. instead of copying files into the project, you could copy them outside the project and then move them into the project (the operation should be faster and may/should generate less events). |
@e2 Thanks for the overview! It fills in a lot of background. I guess I can narrow this issue to just OS X for now.
This is interesting. What I see is that not just the directory is scanned, but all subdirectories also. For a large folder this can take some time. I've already enabled I notice that the Darwin adapter (and BSD, and a few parts of others) pass in "recursive: true" - however, as mentioned, this is not referenced, the action is ALWAYS recursive. What I don't understand though, is why recursive is required? Darwin adapter recursive: true listen/lib/listen/adapter/darwin.rb Line 48 in cba1e51
BSD adapter recursive: true listen/lib/listen/adapter/bsd.rb Line 58 in cba1e51
For example, if I have: If I modify file.rb, the entire tree is crawled for changes. Only the root should be crawled. I guess one problem is that Directory::scan calls back to Change::change for each entry, and in the case of directory this calls Directory::scan again. If I perform a PR to prevent recursion when recursion is not true, and to remove the recursion true from OS X - would that be acceptable? |
Updated comment regarding copying subdir to subdir2 |
Copying and moving is a good point that will need testing. I'll explore that as it may not generate events for subtree entries. In which case I do have another experimental patch that ONLY recurses new folders in order to capture this. You are right though it does seem a minefield of possible inputs and will be hard to optimise without impacting any. I'll report back soon! |
OK - I've been playing around in the root of the watched directory and it seems behaviour here is completely different to that in deeper directories. The record builder places files in the root in the same place as paths are placed. This means So actually seems theres an issue with the way root files are stored in This somewhat explains why recursion is probably implied at the moment. There's no way to check a subfolder was removed without starting a |
@driskell - it's best to handle this case-by-case, or it can be very confusing. One key distinction here: watching recursively is unrelated to scanning recursively. (the first is setting up watched directories, the second is adapter-specific to detect the changes based on events). I think the confusing part is: polling needs recursion (otherwise it can't detect a change deep in the tree, e.g. its scans I'm a bit "scared" to optimize and check OSX, because I'm worried I'll break something not handled by tests. But - if Also, by changing It's tricky - and I don't know enough about how rb-fsevent behaves (which events it gets in which scenarios) to optimize for it. If you can setup a os-agnostic test case (see the The Record is optimized for size somewhat - if you think there's an error there, setup a basic project, modify a file in root and attach a link to the gist with the output (again, I don't have OSX, so I have no idea what rb-fsevent provides). Without a failing test case it's hard to move forward - so let me know if you have a failing example, and I'll help turn it into a test case. As soon as I can reproduce the problem on Linux, I can take over and fix + refactor things. Summary: I don't know how OSX behaves and there are no unit tests which tell me what rb-fsevent events happen in which scenarios. The codepath for OSX and polling maybe share a bit more than they should (ideally, polling should handle recursion by itself - and not create fake dir events going through the whole stack). |
@e2 As I delve deeper I can absolutely agree, case by case basis is best, with sound tests and reproduction cases. What I started out (naively) thinking was fairly straight forward, was certainly not the case. I do firmly believe there's some enhancements to be made here though, and maybe better documentation in code to help. Appreciate the feedback, it's been extremely useful, and I'll take it all away and come back with something cleaner and narrowly targeted 👍
What happens here, is that
Not sure what you mean by Regarding the Thanks again. |
@driskell - I meant "foo" there: if you change Don't worry about creating multiple issues and asking lots of questions - I'm more worried about my changing breaking things for OSX users, so if you can provide cases I can turn into tests on Linux, it would be very, very helpful. If it helps, at some point I wanted to create a "virtual adapter", where you treat it like a filesystem (changing a file, touching a file, removing a directory, moving, etc.) and it returns a set of events for whatever backend you want. You could say it would be like a fakefs adapter. It worked pretty well, but when I got to stubbing rb-fsevent, I realized I have no idea how that adapter behaves (in order to simulate it). That's the biggest issue for me - not knowing exactly how rb-fsevent behaves (which makes it very hard for me to even try to refactor/improve things). For reference, you can check out |
@e2 It doesn't generate an event for That virtual adapter would be a perfect way to do reasonable test cross-platform. Though I guess testing on the platform itself needs doing eventually. I've uploaded a gist regarding the I ran If I perform the same test again but modify You can also see in all of the above, that even though only |
I can also see once a directory is added to |
I've tested it and it didn't work on my simulation, because when files are moved, (e.g. dir1 moved to dir/), the virtual entry doesn't contain entries from the initial build (e.g. initially there's dir1 and dir2, but record['.'] is empty). The "structure" of the record is supposed to be "flatter" (not a tree): record[watched_directory][full_relative_path_to_dir_or_file][dir_or_file_basename] = options (mtime, etc)
record[watched_directory][dir_or_file_basename_in_root] = options (mtime, etc) e.g. record['foo']['bar/baz/bay']['file.txt']
record['foo']['file.txt'] I think the problem here is that dir_entries(watched_dir, '.') should return just a hash with files. |
What was your simulation? In my test Actually, using I guess my patch has made the bad behaviour of subdirectory moves on OS X impact the root as well. I think this is due to the |
@driskell - sorry, you're absolutely right. Having Your patch looks really good btw. |
On Linux, I set the env If you can, setup a small repo with a watched subdirectory - and reproduce problems there, so I can compare with the output I get (I may have bugs in the translation logic, so this may help me find them). I'm not too sure about dealing with the recursion (your other branch) - directory mtime could be used instead, although it may be hard to detect changes without recursion on OSX (because mtime isn't reliable to filter out irrelevant changes). I really appreciate the work you've done on this - thanks!. |
@driskell - I had some additional thoughts and I'm interested in what you think: Current Listen assumptions:
|
Sure. Most of this is from OS X perspective and directory events but I believe would apply elsewhere - testing would be needed though to confirm behaviour, but I would say any directory-event like behaviour that does not match rb-fsevent for OS X would be somewhat intuitive or essentially useless for the purpose listen needs it for.
Hope I'm not direct in some of my thoughts, but I hope it helps in decisions and planning. As mentioned I'm happy to help out with any bits - and hopefully will be better suited to help once I've played with the acceptance testing more so rather than rambling I can actually provide reproducing code! |
Polling "assumes" every directory has been modified - so it shares the scanning logic with Darwin.
I'm not sure this will play well if a file is removed and a directory is created with the same name (or vice verse) Or, if directory
What events to do get if a directory tree is added? What about an empty directory?
Is it necessary for added directory trees? (related to above question about events)
Mostly because Record uses Some optimizations you mention may need sequential access, or they may be too complex to implement - especially to work reliably with JRuby and Rubinius.
And I don't think this is a good thing. Itm makes the code very complex, difficult to maintain and debug and quite hard to understand. The performance gain from concurency on MRI is minimal (scanning is CPU bound, not IO-bound as one would expect).
Polling should be optimizable on high-precision-mtime filesystems (anything except FAT and HFS). This is important for filesystems where inotify or FSEvent are unavailable. At the very least, optimizing Darwin adapter shouldn't slow down Polling (and e.g. Polling doesn't need directory info in Record if it's indiscriminately recursive).
Polling probably should have less in common (in the codebase) with Darwin than it has now. The API should reflect more what happens than using magic "existing-or-not-existing" parameters.
That shouldn't be too difficult, given specs are added to Record.
I'm not sure - is FSEvent providing insufficient events? (e.g. when directories are added). Otherwise, having directories is a Darwin-specific features - and so maybe there should be a Darwin-specific Record instead? Also, it may help to populate this "extra" Record at startup (to reduce the need for scanning on the first run).
mtime is stored in the directory (usually?) - so changing a file does change the directory.
Accessing the Record may be slow (the calls go through Celluloid mailboxes), and Polling uses Directory.scan (and doesn't store directory info - since it doesn't need it because of recursion). There are also symlinks, which can add lots of other cases - calling
I'd say less likely - both Windows and Linux adapters use file events, and they do rely on the Record in some cases anyway (so there wouldn't be a difference). I translate file-events into dir events for simulation - so you can pick which events should be handled as file events, and which as dir events.
It's too easy to break something, and there are not enough tests to really considering optimizing. "Works" is better than "fast but broken", especially with so many users relying on Listen. Crashing is much better than not-responding to changes. And failing tests are much better than crashing.
It's a complex and confusing topic - and we're both missing information, so neither of us can see the big picture. If you have any suggestions (minor or major) that would help, let me know. The current code isn't good at all - so anything that confused you or didn't work as advertised should be changed. |
@driskell - I've updated,rebased and squashed: #308 If you can, try it out. If you have any changes you'd like on top, just attach links to the commits. Major changes:
The changes are pretty drastic, and even though all the tests pass, this doesn't mean things work. |
Thanks I'll give that a go. On the previous:
This is a good spot. This would reach File::change which currently assumes Record entries to be for files only. So it needs to be updated to account for existing entry not being a file. Without the change, we don't detect folder removals if they are moved, so this needs to happen I believe. It will likely mean Record::add_dir then needs to verify if the existing entry is a file or not and fix it up. I can see this is deepening somewhat - and fixing this moving folder problem may be a large task!
Just tested this scenario and it is as expected. A directory change event is raised for the parent directory in both cases - no even is raised for the new directory in either cases. If files are copied in, however, you receive events for each directory as files are added (but not for an empty directory as it never has files added.)
Yes, this is what I mean by "not always necessary" - recursion into a subfolder is ONLY needed if that subfolder is detected as new (or if Record remembers it as a file for example) or if the subfolder used to be there and isn't we should recurse the Record tree. Interestingly, back to your point about file becoming a folder - we now have a potential blocker - if a folder becomes a file, we need to maintain existing Record contents before file overwrites it. This would have to be handled carefully when File::change detects an entry used to be a directory, maybe even maintaining reference to the Record dir_entries and passing to Directory::scan - plot thickens here!
I'm still not sure the problem with ||=. Storing directories is nothing different than storing files is it not?. The only difference is the data stored with the name entry. One is the data for a file, one is the data for a directory. To distinguish, a :type is stored or simply directories have :mtime missing which seems to possibly have been past behaviour as there is a reference to this in the old Directory::dir_entries
Not really. A subfolder is moved to trash for example - we get an event for the parent folder where that folder used to be. We have to work out what the change was in that folder - how do we work out if it was a moved away folder, and what it name was, if we don't remember it?
I've tested on Mac and Linux. Modifying a file in a directory does not change the directory. (Using an editor may well do so, however, as most editors create a new file, delete the old, and then rename - which in essence generated three directory changes.) Specifically, mtime only changes for a directory when the directory list changes. Individual file content changes don't impact it as the list remains the same.
May be open for discussion this. I would say symlinks should be treated as files. Otherwise, we're watching a directory potentially outside the watched root.
I appreciate this and perfectly understand the need for stringent testing, and our discussions I do hope are helping create a good log of understanding for anyone else wondering about the internals and what does what. It's definitely helped me appreciate the code a lot more, but also see some room for improvement. I agree with you here mostly. However, I would say it currently does not reliably work for OS X / directory-events, due to the missing directory lists in Record that mean moved folders are not raising the correct remove events. Preventing recursion I would say is n optimisation to drop for now and look at later, once the actual problems are solved. |
It's related to logic regarding directories, because "empty" may mean "empty directory" or "not scanned yet". Also,
rb-inotify works only on directories (BSD use kqueue to actually watch files). rb-inotify gets a list of directories to watch - and reports changes to files. This is basically via the
I mentioned that to suggest that you don't put too much faith in the existing code (respecting your time). Expect the existing code to be buggy and "prove" it with test cases. Once we run out of things to test, we can release.
My feelings are the opposite, lol! Let me know if you're ready to move ahead, or if you have a specific error you want to fix (or need help writing a test for). |
@driskell - I'd like to resurrect this (PR #308 updated). I've tried to simulate the rb-fsevent adapter as close as possible on Linux. I'd take the approach of disabling recursion (now possible with the So until all issues are sorted out, people could "opt-out" of recursion (to get the performance boost) and by disabling recursion, meaningful work and testing could be done to improve things. If something doesn't work, there should be failing tests anyway (which can lead to quick fixes). So if you can create a failing example, and I can reproduce it as a test case on Linux, I can take a stab at getting directory info in the record, adding missing tests, etc. There are a lot of changes (based on your fork) - and quite a bit of refactoring I need to do (the whole recursion thing should be object oriented - so it can be tested without having to understand the full stack). Any step-by-step suggestions from your side would be very helpful - feel free to open a "Refactor: " issue with a list of changes you think would help (so we can track this separate from this issue) - or open a PR, so I can keep track of what works for you. Thanks! |
This issue is over 6 years old, so I'm closing it. |
Hello,
On OS X I've noticed recursion is used when an event is received. So even though events are received on a per folder basis it will recurse into them. This means if you copy in a large tree of files, it will recurse the tree exponentially dependent on depth. For example a simple 1/2/3/4/5 would mean a recursion over the 4/5 layer 3 times. It also means a single change to a file in the root triggers a recursion of the entire watched folder. If the folder contains thousands of folders/files it will then delay the next change by up to 10-15 seconds sometimes in the tests I've done while the CPU sits at 100%.
Is there a reason for this recursion? Do any of the adapters (except poll) even need this at all?
(On a separate note - I notice recursion is implied... the actual recursive flag passed by the adapters isn't even referenced.)
There is one reason I can think - and that is if a new folder is added - it would need to Directory.scan it in order to process its contents, thus it would (kind of) need to recurse into it. This can easily be made a special case, and full recursion only performed if requested (by polling adapter for example.)
Does this make sense? Have I misunderstood the code?
The text was updated successfully, but these errors were encountered: