paulmillr / chokidar

Minimal and efficient cross-platform file watching library
https://paulmillr.com
MIT License
10.86k stars 575 forks source link

Potential resource leak in unwatch() - seen in getWatched() #1275

Closed aicioara closed 2 months ago

aicioara commented 1 year ago

Describe the bug

Background: I have a long running process (days/months) that will dynamically add and remove dirs to watch. I use .add() and .unwatch() for the two operations and getWatched() to retrieve the list of dirst watched at any given moment.

Problem: .unwatch() does not remove the files from the list that getWatched() returns, but the files are indeed not matched anymore.

The problem is two fold:

  1. The list returned by getWatched() is not reliable
  2. The internal this._watched Map will continue to grow over the lifespan of the process

Versions:

To Reproduce:

const chokidar = require('chokidar');
const watcher = new chokidar.FSWatcher({}).on('all', (event, path) => {
    console.log(event, path);
});

let WATCHED_PATH = null
function changePath(path) {
    if (WATCHED_PATH) {
        watcher.unwatch(WATCHED_PATH)
    }
    WATCHED_PATH = path
    watcher.add(WATCHED_PATH)
}

setTimeout(() => changePath('/tmp/path1'), 100)
setTimeout(() => changePath('/tmp/path2'), 200)
setTimeout(() => console.log(watcher.getWatched()), 300)

Expected behavior

{
  '/tmp': [ 'path2' ],
  '/tmp/path2': [ '11', '12', '13' ]
}

Actual behavior

{
  '/tmp': [ 'path2' ],
  '/tmp/path1': [ '1', '2', '3' ],
  '/tmp/path2': [ '11', '12', '13' ]
}

Additional context

Again, the actual behavior of the watching (the important part) is correct, just the output of getWatched() is incorrect. I believe that happens because of the use of ignored paths.

https://github.com/paulmillr/chokidar/blob/0f163b89f3ae76607900aa48fec9fa3fdefd7ca1/index.js#L485-L488

While I could technically ignore the output of getWatched(), or even better, patch the library with a

clean() {
  this._watched.clear()
}

My main concern is that the long running process may leak arbitrary resources (file watchers, etc) across the system.

paulmillr commented 1 year ago

Right. I think removing glob support altogether will solve this if v4 is released.

aicioara commented 1 year ago

If I understand correctly, this means that a fix will only be available in v4, correct?

I am running this with disableGlobbing: true, depth: 0 options already, but that does not appear to fix it.

paulmillr commented 1 year ago

If I understand correctly, this means that a fix will only be available in v4, correct?

I don't focus much on Chokidar these days, so unless someone contributes a fix, yes.

aicioara commented 1 year ago

Thank you for clarifying. At the moment, I am using the following workaround, but I may take a stab at it if I get the bandwidth. I am not sure what are the drawbacks of my approach or whether any resources leak.

const chokidar = require('chokidar');

function newFSWatcher() {
    return new chokidar.FSWatcher({}).on('all', (event, path) => {
        console.log(event, path);
    });
}

let watcher = newFSWatcher()

let WATCHED_PATH = null
async function changePath(path) {
    if (WATCHED_PATH) {
        await watcher.close()
        watcher = newFSWatcher()
    }
    WATCHED_PATH = path
    watcher.add(WATCHED_PATH)
}

setTimeout(() => changePath('/tmp/path1'), 100)
setTimeout(() => changePath('/tmp/path2'), 200)
setTimeout(() => console.log(watcher.getWatched()), 300)