fs: fix nonNativeWatcher watching folder with existing files

[MoLow]

a simple case that has not worked before this fix:

const fs = require('fs');
const watcher = fs.watch(__dirname, { recursive: true });
watcher.on('change', (event, filename) => {
  console.log(event, filename);
});

setTimeout(() => fs.writeFileSync(__filename, fs.readFileSync(__filename)), 1000);

[MoLow]

CC @nodejs/fs

[MoLow]

@anonrig I was able to find the inconsistency on macOS:
it seems that a file is considered renamed if it is created in the last 20 seconds

const fs = require('fs');
const path = require('path');


setInterval(() => fs.writeFileSync(__filename, fs.readFileSync(__filename)), 2000);

const watcher = fs.watch(__dirname, { recursive: true });
watcher.on('change', (event, filename) => {
  if (filename === path.basename(__filename))
    console.log(event, Date.now() - fs.statSync(filename).birthtimeMs);
});

outputs:

./node a.js
rename 3191.91748046875
rename 5191.91748046875
rename 7192.91748046875
rename 9192.91748046875
rename 11193.91748046875
rename 13194.91748046875
rename 15195.91748046875
rename 17195.91748046875
rename 19197.91748046875
rename 21198.91748046875
change 23199.91748046875
change 25200.91748046875
change 27201.91748046875

[anonrig]

Isn't this boolean expression better for detecting the creation of files?

currentStats.birthTimeMs !== 0 && previousStats.birthtimeMs === 0

[anonrig]

@anonrig I was able to find the inconsistency on macOS: it seems that a file is considered renamed if it is created in the last 20 seconds

@MoLow This code does not produce rename on macOS with node 19 for me.

➜  node git:(perf/stream-encoding) ✗ node test.js
change 10095.632568359375
change 12096.632568359375
change 14097.632568359375
change 16098.632568359375
change 18100.632568359375
change 20100.632568359375
change 22100.632568359375
change 24101.632568359375
change 26102.632568359375
change 28102.632568359375
change 30104.632568359375
change 32104.632568359375
change 34105.632568359375
change 36106.632568359375
change 38105.632568359375

You are using ./node. I assume this is your personal build. Can you retry using the distributed node and validate if you receive the same response?

[MoLow]

This code does not produce rename on macOS with node 19 for me.

@anonrig this reproduces only in the case where the file is "new" IE created recently,
besides here you said macOS only fires rename now you say it only fires change - so you agree it is inconsistent :)

[anonrig]

@anonrig this reproduces only in the case where the file is "new" IE created recently, besides here you said macOS only fires rename now you say it only fires change - so you agree it is inconsistent :)

@MoLow I dug into the Node code, as it seems if when the file is newly created, it triggers a rename event, but after a while, it is changed to a change event. For me, it was 10 seconds, although for you, it was 20 seconds. (On a different run, I also had a 20 second delay). For this particular reason (inconsistency) my thoughts are to be persistent, and emit a change event only.

I looked into both libuv code and kqueue docs. I think we should open an issue to libuv to understand the reasoning behind this, and document this on the Node docs.

Meanwhile, I recommend, only emitting change event, add a comment to the particular line that throws change and just merging this pull request, for the sake of unblocking development. WDYT @MoLow?

PS: My thoughts are: Let's keep the test as it is right now.

[MoLow]

@anonrig I have reverted the condition, can you approve?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/48206/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/48207/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/48211/

[nodejs-github-bot]

Landed in 147d810

[danielleadams]

@MoLow, same here - the test couldn't be landed in v18.x-staging. Do you mind creating a backport PR here too?