Recursive remove watches when recursion disabled

Issue #59 resolved
Mihail Slobodyanuk
created an issue

Hi, Martín! The issue observed on Linux with Inotify watcher. I use not recursive mode of EFSW and made external recursion to do some filtering of observed directories. When top directory removed i catch delete event only for it. Now i need remove watches for all children. But i don't know what child directories was in deleted directory. I investigege code and found recursive deletion only for recursive watch mode: https://bitbucket.org/SpartanJ/efsw/src/9a7cbec70b8a88b2e876a382f57c59f3796da0d9/src/efsw/FileWatcherInotify.cpp?at=default&fileviewer=file-view-default#FileWatcherInotify.cpp-477

I am sure it's wrong in the inotify case. Need always delete watches recursive. If some watched directory deleted and then created new directory with same name then old watcher will not deliver events for new one.

I'll made additional investigation for another OSes and let you know results Thanks

Comments (6)

  1. Martín Lucas Golini repo owner

    Mihail, i cannot reproduce the bug. I thought it made sense what you said but if you add the watchers manually they report their own deletion, since they are registered as an inotify watch, in the same way that in the recursive watcher. The code on your repository will produce a double remove in the watches, so i don't understand what's your use case that it's failing.

    I'll gladly take a look at an example demonstrating the failure. I can send me an example i'll debug it and try to find the correct fix.

    Thanks,

    Regards

  2. Mihail Slobodyanuk reporter

    Here is the test:

    #include <efsw/efsw.hpp>
    #include <efsw/System.hpp>
    #include <efsw/FileSystem.hpp>
    #include <signal.h>
    #include <iostream>
    #include <sys/stat.h>
    #include <unistd.h>
    #include <stdlib.h>
    #include <stdio.h>
    
    bool STOP = false;
    
    void sigend(int signal)
    {
        std::cout << std::endl << "Bye bye" << std::endl;
        STOP = true;
    }
    
    efsw::FileWatcher fileWatcher;
    
    void addWatch(const char* path);
    
    /// Processes a file action
    class UpdateListener : public efsw::FileWatchListener
    {
        public:
            UpdateListener() {}
    
            std::string getActionName( efsw::Action action )
            {
                switch ( action )
                {
                    case efsw::Actions::Add:        return "Add";
                    case efsw::Actions::Modified:   return "Modified";
                    case efsw::Actions::Delete:     return "Delete";
                    case efsw::Actions::Moved:      return "Moved";
                    default:                        return "Bad Action";
                }
            }
    
            void handleFileAction( efsw::WatchID watchid, const std::string& dir, const std::string& filename, efsw::Action action, std::string oldFilename = ""  )
            {
                std::cout << dir + ( oldFilename.empty() ? "" : "from file " + oldFilename + " to " ) + filename + " has event " << getActionName( action ) << std::endl;
                std::string path = dir + filename;
                switch ( action )
                {
                    case efsw::Actions::Add:
                    case efsw::Actions::Modified:
                    case efsw::Actions::Moved:
                        if ( efsw::FileSystem::isDirectory( path ) ) 
                        {
                            addWatch(path.c_str());
                        }
                        break;
                    case efsw::Actions::Delete:
                        {
                            //He we can't make recursive remove because can't enumerate already removed children
                            efsw::FileSystem::dirAddSlashAtEnd( path );
                            //Makes sense move this call to handleFileAction caller because this watch invalidated
                            fileWatcher.removeWatch(path);
                        }
                        break;
                }
            }
    } ul;
    
    //Custom predicate do we need watch directory
    bool needWatch(const char* path){return true;}
    
    //Custom function to make recursive watch
    void addWatch(const char* path) {
        if(!needWatch(path)) return;
    
        efsw::WatchID err;
    
        if ( ( err = fileWatcher.addWatch( path, &ul, false ) ) > 0 )
        {
            fileWatcher.watch();
    
            std::cout << "Watching directory: " << path << std::endl;
        }
        else
        {
            std::cout << "Error trying to watch directory: " << path << ". ";
            std::cout << efsw::Errors::Log::getLastErrorLog().c_str() << std::endl;
        }
    
        std::map<std::string, efsw::FileInfo> files = efsw::FileSystem::filesInfoFromPath( path );
        std::map<std::string, efsw::FileInfo>::iterator it = files.begin();
    
        for ( ; it != files.end(); it++ )
        {
            efsw::FileInfo fi = it->second;
    
            if ( fi.isDirectory() && fi.isReadable() )
            {
                addWatch(fi.Filepath.c_str());
            }
        }
    }
    
    int main(int argc, char **argv)
    {
        signal( SIGABRT ,   sigend );
        signal( SIGINT  ,   sigend );
        signal( SIGTERM ,   sigend );
    
        std::cout << "Press ^C to exit demo" << std::endl;
    
        system("rm -rf test");
        mkdir("test", ACCESSPERMS);
        mkdir("test/level1", ACCESSPERMS); // Create subdir and create watch for it in UpdateListener
        mkdir("test/level1/level2", ACCESSPERMS); // Create subdir and create watch for it in UpdateListener
        addWatch("test");// Watch top level
        efsw::System::sleep( 100 );
        //Rename and remove subdirs
        rename("test/level1", "level1"); //No delete test/level1/level2 event as well
        system("rm -rf level1");
        //Re-create sub-dirs
        mkdir("test/level1", ACCESSPERMS);
        mkdir("test/level1/level2", ACCESSPERMS);//Watch for old test/level1/level2 is present and can't be created for new one
        efsw::System::sleep( 100 );
        mkdir("test/level1/level2/level3", ACCESSPERMS); // This creation is not detected
    
        while( !STOP )
        {
            efsw::System::sleep( 100 );
        }
    
        return 0;
    }
    

    Is it any chance add some Unit tests engine to project to make this things simpler?

  3. Martín Lucas Golini repo owner

    Hi Mihail!

    As i said before, your patch breaks even a simple delete with recursive watches. So, it was not a valid solution.

    Now that i could see the actual failure i was able to patch it to behave as expected.

    Also, yes, the project needs without any doubt unit testing, but i need time to prepare them, and right know i don't actually have it. Also it'll be a challenge to test this since, there are minor difference between every platform that will prevent to have an exact same result on all platforms. And there are a lot of corner cases. This is actually something that it's impossible to fix, so may be i'll need to create some platform specific unit tests for those cases.

  4. Log in to comment