fs#opendir does not return all files in subdirectories when bufferSize is less than the directory size

[mdouglass]

Version

Node.js v20.4.0

Platform

Linux rowlf 6.3.11-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Sun Jul 2 13:17:31 UTC 2023 x86_64 GNU/Linux

Subsystem

fs

What steps will reproduce the bug?

If I call fs#opendir in recursive mode on a directory structure where a subdirectory has more than bufferSize files in it, than the complete results will not be returned (each subdirectory gets truncated to bufferSize entries.

I have attached a code sample that demonstrates the bug. It requires a directory structure with at least 64 files in a subdirectory of a directory named to-read:

.
├── repro-opendir.mjs
└── to-read
    └── subdir
        ├── 1
        ├── 2
        ├── ...
        └── 64

I created the above on my machine with for i in $(seq 1 64); do; touch $i; done

repro-opendir.mjs:

import assert from "assert";
import fs from "fs/promises";

async function opendir(dir, bufferSize) {
  const files = [];
  for await (const file of await fs.opendir(dir, { recursive: true, bufferSize })) {
    files.push(file);
  }
  return files;
}

async function main() {
  const filesA = await opendir("./to-read", 128);
  assert.strictEqual(filesA.length, 65);

  const filesB = await opendir("./to-read", 32);
  assert.strictEqual(filesB.length, 65);
}

await main();

How often does it reproduce? Is there a required condition?

With the directory structure as described it reproduces 100% on node.js v20.3 and v20.4. I have not tried other versions.

What is the expected behavior? Why is that the expected behavior?

Neither assertion should trigger.

What do you see instead?

With the above code sample, the second assert will fail with the following message:

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

33 !== 65

    at main (file:///home/matthew/spikes/repro-node-opendir/repro-opendir.mjs:17:10)
    at async file:///home/matthew/spikes/repro-node-opendir/repro-opendir.mjs:20:1

Additional information

No response

[mdouglass]

It looks like the problem here is caused by readSyncRecursive not looping on all results returned by the sub-handle. I added a draft pull request #48829 that naively loops on it in place but I'm not sure if there are issues with blocking the event loop for a large subdirectory if it is done that way.

original:

node/lib/internal/fs/dir.js

Lines 172 to 181 in 12a93ce

	const result = handle.read(
	this[kDirOptions].encoding,
	this[kDirOptions].bufferSize,
	undefined,
	ctx,
	);
	if (result) {
	this.processReadResult(dirent.path, result);
	}

revised:

node/lib/internal/fs/dir.js

Lines 172 to 185 in 11cedfe

	while (true) {
	const result = handle.read(
	this[kDirOptions].encoding,
	this[kDirOptions].bufferSize,
	undefined,
	ctx,
	);
	if (result) {
	this.processReadResult(dirent.path, result);
	} else {
	break;
	}
	}

Tagging @Ethan-Arrowood as you seem to have been here most recently.

[Ethan-Arrowood]

Yeah this was identified recently - i have a PR fixing it differently: #48698

WDYT?

[mdouglass]

I tried your branch out and it does not fix my issue. I think the problem is because opendir and readdir don't share the same code paths. On the upside, your branch pointed me to where I can change a test to illustrate the issue. I will update my PR branch with a change to test/sequential/test-fs-opendir-recursive.js that demonstrates my bug.

[Ethan-Arrowood]

Okay cool. I'll focus on finishing up the change necessary for readdir, and then we can use your work for opendir.

[mdouglass]

This shows the test change that triggers my issue 9483314

[mdouglass]

Any update on this? The issue is still reproducible in v20.8.0 and my proposed fix above still seems reasonable?

[Ethan-Arrowood]

Interesting. I assumed #49603 would fix your issue. Feel free to send a PR with another fix though and we can get it landed asap!

[BridgeAR]

@mdouglass I saw you have a draft open that would just need an update?

[mdouglass]

I haven't touched it as I was still awaiting feedback on the questions I raised in the original PR.

It looks like the problem here is caused by readSyncRecursive not looping on all results returned by the sub-handle. I added a draft pull request #48829 that naively loops on it in place but I'm not sure if there are issues with blocking the event loop for a large subdirectory if it is done that way.

[juanarbol]

Besides a bug, this may be a lack of documentation.

juanarbol@b35181f#diff-5a0fa708afa6a2202dc64e5038e16c8e249cb1c3ec23c87885acc2a662a1ec00L48

It used to read up to 32 folders, now, it can read as bufferSize (can hold).

https://github.com/juanarbol/node/blob/main/lib/internal/fs/dir.js#L52

BufferSize will be 32 by default, and overwritten by the provided args.

[sjohannes]

This bug specifically happens with {recursive:true}; any subdirectory containing more items than bufferSize will get truncated. The following is a reproducer to demonstrate using bufferSize of 1 (note: it will leave an issue48820 directory in cwd):

import { mkdir, open, opendir, rmdir } from "node:fs/promises";

await mkdir("issue48820");
await mkdir("issue48820/1");
await mkdir("issue48820/1/1");
await mkdir("issue48820/1/1/1");
await mkdir("issue48820/1/1/2");
await mkdir("issue48820/1/2");
await mkdir("issue48820/2");

const dir = await opendir("issue48820", { bufferSize: 1, recursive: true });
for await (const dirent of dir)
  console.log(`${dirent.parentPath}/${dirent.name}`);

// Actual output:
//   issue48820/1
//   issue48820/1/1
//   issue48820/1/1/1
//   issue48820/2

In the root directory both items are found. However, in each subdirectory only one is found.

@Ethan-Arrowood implemented the recursive option; maybe he has more insights. (Edit: oops, he's already in this thread.)

[Ethan-Arrowood]

Okay I think I am understanding the issue. But there is a lack of documentation on bufferSize. I guess my question is, is this value simply an internal detail? Like does it mean, "still let me iterate over all items within a directory, but only read these many into memory at any given time?" Or does it mean, "only give me up to this number of entries within the directory?"

[Ethan-Arrowood]

Okay, I've added an assertion to the opendir recursive test file that proves this bug:

diff --git a/test/sequential/test-fs-opendir-recursive.js b/test/sequential/test-fs-opendir-recursive.js
index a7e9b9a318..4328f1f4c1 100644
--- a/test/sequential/test-fs-opendir-recursive.js
+++ b/test/sequential/test-fs-opendir-recursive.js
@@ -132,6 +132,7 @@ function getDirentPath(dirent) {
 }
 
 function assertDirents(dirents) {
+  assert.strictEqual(dirents.length, expected.length);
   dirents.sort((a, b) => (getDirentPath(a) < getDirentPath(b) ? -1 : 1));
   assert.deepStrictEqual(
     dirents.map((dirent) => {
@@ -221,3 +222,10 @@ function processDirCb(dir, cb) {
 
   test().then(common.mustCall());
 }
+
+// BufferSize
+{
+  const dir = fs.opendirSync(testDir, { bufferSize: 1, recursive: true });
+  processDirSync(dir);
+  dir.closeSync();
+}

Will fix here: #55744

[Ethan-Arrowood]

How the buffering works is that in a non-recursive mode, the first read operation will fill the buffer up to that limit. and then reads will be pulled from that buffer until its empty, and then it will do another read operation, and repeat this process until the entire handle is read. This works fine because its all handled at the C level. That is how it knows where to pick up again when it needs to read 1+ times.

But in recursive mode, the implementation is only at the JS level. when a read happens, it is breadth first. Meaning like reading the root / first returns all things within that directory. As those dirents are returned to the user, if we detect its a directory, then and only then do we read that directory which happens immediately. The nested read opens a new handle on that sub directory, say /a/, and inherits the bufferSize optoin. So on that first read, it will fill the Dirent buffer with the results of /a/, but then it abandons the handle and may never completely read it.

This issue has highlighted a fundamental issue with the implementation. Fixing it will require a bit of work to do, but it shouldn't be hard. I'll have a fix soon.

Maybe an example?

Lets say you have a bufferSize of 2, and the following filesystem:

/
|- a/
|    |- 1
|    |- 2
|    |- 3
|- b/
|    |- 1
|    |- 2
|    |- 3

let d = opendir('/', { recursive: true, bufferSize: 2 }) will create a handle on /.

Then when the user calls d.read() the first time, the instance:

1. reads the top level and fills the internal buffer with 'a', 'b'
2. prepares to return the first item 'a'
3. detects that /a is a directory itself and calls readSyncRecursive
4. that read call immediately opens a handle on /a and reads from it, inheriting the bufferSize value of 2
5. that operation returns /a/1 and /a/2, those are pushed to the Dir buffer
6. the handle on /a is then closed and thrown away
7. finally /a is returned for the original d.read() operation

The next call to d.read() will follow a similar flow for the next entry in the list 'b'. At the end of this call, the internal buffer now has '/a/1', '/a/2', '/b/1', '/b/2'

The next call to d.read() returns '/a/1'. That is a file so nothing else is done. But the issue has already occurred. When a was read, we didn't actually fully read it.

Thus, I need to completely rework the implementation here to handle all of this.

[Ethan-Arrowood]

PR is ready for review 😄

It uses the naive solution @mdouglass shared above. I think this okay so that we get the bug fixed immediately. We can improve the efficiency / implementation as a follow up IMO.

[mdouglass]

Thanks @Ethan-Arrowood, this lgtm.

[Ethan-Arrowood]

PR merged 🚀

[tjaisson]

May I suggest that a warning be added to the documentation ?
It would have been useful to me.

[Ethan-Arrowood]

That would be a great addition until #55764 is fixed. Would you like to send a PR?

[tjaisson]

I'm not very used to doing this but here is a suggestion anyway #55876