Skip to content
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

File blob is not invalidated if the file was initially empty then written to #47161

Closed
Jamesernator opened this issue Mar 19, 2023 · 1 comment · Fixed by #47199
Closed

File blob is not invalidated if the file was initially empty then written to #47161

Jamesernator opened this issue Mar 19, 2023 · 1 comment · Fixed by #47199
Labels
fs Issues and PRs related to the fs subsystem / file system.

Comments

@Jamesernator
Copy link

Jamesernator commented Mar 19, 2023

Version

v19.8.1

Platform

Linux 5.19.0-35-generic #36~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Fri Feb 17 15:17:25 UTC 2 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

fs

What steps will reproduce the bug?

The following should produce an exception when reading the blob, but currently doesn't:

import fs from "node:fs";

// Create an empty file
fs.writeFileSync("./test.txt", "");

const blob = await fs.openAsBlob("./test.txt");

// write to the file, which should invalidate the blob
fs.writeFileSync("./test.txt", "foobar");

// This should throw an error, but returns the empty string instead
const result = await blob.text();

console.log(JSON.stringify(result)); 

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

This happens consistently.

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

It should throw a DOMException, this works fine if the file was initially not empty, i.e. this throws an error as expected:

import fs from "node:fs";

// Create an empty file
fs.writeFileSync("./test.txt", "NOT_EMPTY");

const blob = await fs.openAsBlob("./test.txt");

// write to the file, which should invalidate the blob
fs.writeFileSync("./test.txt", "foobar");

// This should throw an error, but returns the empty string instead
const result = await blob.text();

console.log(JSON.stringify(result)); 

What do you see instead?

No exception is thrown.

Additional information

The Chrome behaviour for empty files being read after modification is the same as for non-empty files — i.e. an error is thrown in all cases.

(Curiously the Firefox behaviour allows reading the latest contents of the file regardless of modification status, however this seems buggy as the blob .size never actually changes).

@bnoordhuis
Copy link
Member

It's probably because of this check (.text() calls .arrayBuffer()):

node/lib/internal/blob.js

Lines 269 to 271 in 0fd14e4

if (this.size === 0) {
return PromiseResolve(new ArrayBuffer(0));
}

And it's arguably wrong because special files (like files under /proc on linux) are often "zero-sized" while actually containing readable data.

There's another instance of that pattern inside the stream() method:

node/lib/internal/blob.js

Lines 319 to 323 in 0fd14e4

if (this.size === 0) {
return new lazyReadableStream({
start(c) { c.close(); },
});
}

Pull request welcome.

@bnoordhuis bnoordhuis added the fs Issues and PRs related to the fs subsystem / file system. label Mar 19, 2023
debadree25 added a commit to debadree25/node that referenced this issue Mar 21, 2023
nodejs-github-bot pushed a commit that referenced this issue Mar 23, 2023
Fixes: #47161
PR-URL: #47199
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
RafaelGSS pushed a commit that referenced this issue Apr 5, 2023
Fixes: #47161
PR-URL: #47199
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
RafaelGSS pushed a commit that referenced this issue Apr 7, 2023
Fixes: #47161
PR-URL: #47199
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants