lib: call normalizeEncoding in fs.writeFileSync and fs.readFileSync - #60539
lib: call normalizeEncoding in fs.writeFileSync and fs.readFileSync#60539JonasBa wants to merge 1 commit into
Conversation
|
Review requested:
|
9143536 to
121725f
Compare
There was a problem hiding this comment.
This breaks default encoding to be buffer
normalizeEncoding normalizes undefined to 'utf8', but readFileSync does not set options.encoding to a default value at that point, unlike writeFileSync, and the expectation is to read Buffers by default, not strings
121725f to
862df86
Compare
862df86 to
771bd39
Compare
Codecov Reportβ
All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #60539 +/- ##
==========================================
- Coverage 88.58% 88.56% -0.03%
==========================================
Files 704 704
Lines 207839 207844 +5
Branches 40051 40043 -8
==========================================
- Hits 184117 184076 -41
- Misses 15791 15809 +18
- Partials 7931 7959 +28
π New features to boost your workflow:
|
|
CI is going to fail unless #60552 is fixed first, which is though unrelated directly Basically, #60552 fix has to go in first. (Upd: #60553) |
|
@ChALkeR, I appreciate the thorough review and explanation here, thank you so much! |
Saves already replaced data.json by renaming a temp file over it, which
is enough for a crash: a process that dies has no say in what the kernel
does with writes it already accepted. It is not enough for a power cut.
The rename is metadata and the contents are data, and the two reach the
platter independently β the directory entry can be durable while the
bytes it points at are not, which resolves after reboot as a
present-but-empty data.json. The whole file, gone, from a save that
reported success.
So the writer now flushes the contents with `fsyncSync` before the
rename and the directory after it. On macOS both are `F_FULLFSYNC`,
Apple's real flush-to-permanent-storage primitive rather than the one
that lies for speed. Roughly 30β60 ms per save against a 500 ms
debounce, so nobody will meet it.
The payload is handed over as a Buffer, deliberately. `writeFileSync`
with `{flush: true}` looks like it would do all of this in one call and
for a utf8 string silently skips the fsync entirely (nodejs/node#60539)
β the C++ fast path drops the option. A write that quietly isn't durable
is worse than one that never claimed to be. The directory flush is
best-effort for the opposite reason: by the time it runs the data is
already safe, so throwing there would report a successful save as a
failure, stop the in-memory document advancing, and turn the next save
into a conflict nobody can resolve.
It lives in its own module because the dependency graph forbids anything
else: storage imports paths imports config, so config β which records
where the data folder is, and is worth exactly as much as the data β
could not have imported this from storage without a cycle. It now goes
through it too.
Two things the new config tests turned up, both fixed here. `writeConfig`
applied none of the validation `readConfig` does, so a relative dataDir
would be used by the session that wrote it and dropped by every session
after β the two disagreeing about where the records live. And unknown
keys were dropped on read, which matters now that every setting is
changed by spreading the current config: running an older Casebook once
would strip a newer one's settings, and the self-updater makes that a
thing that can happen.
Fixes #49888 by adding calls to
normalizeEncoding.Benchmarks between #f46152fdb3 and CL from this PR
I am open to suggestions here, but given that in the case of fast paths, the only encoding we care about is utf8, would it perhaps make more sense to have a
isUTF8Encoding()which would return a bool?Reviews and suggestions are much appreciated ππΌ