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

Fix atomic createWriteStream #8194

Merged
merged 7 commits into from
Jun 13, 2022
Merged

Fix atomic createWriteStream #8194

merged 7 commits into from
Jun 13, 2022

Conversation

mischnic
Copy link
Member

@mischnic mischnic commented Jun 7, 2022

Closes #7813

I think this is also the cause of at least some CI flakiness (for tests that use the NodeFS as the output FS).

Problem

(copied from the linked issue)

The problem is that when writing the bundles

await new Promise((resolve, reject) =>
pipeline(
res.stream,
outputFS.createWriteStream(

this rename here

isn't awaited but happens sometime afterwards asynchronously. (The whole handleClose callback is called late, it's not just the missing fs.rename callback)
But it doesn't happen at all if Parcel stops the process forcefully:

process.exit(exitCode);

Probable cause

Node 13.14.0 is working correctly, the next version 14.0.0 has this behaviour. Something regarding the timing of the close event on the createWriteStream() stream has changed.

Potentially nodejs/node@e13a37e49d

Solution

So with my limited understanding of Node streams, I don't know if implementing an atomic write stream is even possible (anymore). So the easy (but brute force) solution is to let the callee do call the move at the right time (which worked out for all usages we have). E.g.

It looks like the problem was that fs-write-stream-atomic added a wrapper Writeable "proxy" over the actual fs write stream. Overriding the fs.close used by Node's createWriteStream to additionally move the files afterwards works

@parcel-benchmark
Copy link

parcel-benchmark commented Jun 7, 2022

Benchmark Results

Kitchen Sink ✅

Timings

Description Time Difference
Cold 1.37s +1.37s ⚠️
Cached 310.00ms +311.00ms ⚠️

Cold Bundles

No bundles found, this is probably a failed build...

Cached Bundles

No bundles found, this is probably a failed build...

React HackerNews ✅

Timings

Description Time Difference
Cold 8.54s -67.00ms
Cached 416.00ms +19.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

AtlasKit Editor ✅

Timings

Description Time Difference
Cold 1.40m -899.00ms
Cached 2.24s -35.00ms

Cold Bundles

Bundle Size Difference Time Difference
dist/16.069344b7.js 905.00b +0.00b 30.95s -3.60s 🚀
dist/simpleHasher.46d6f2e5.js 742.00b +0.00b 34.29s -28.64s 🚀
dist/index.html 240.00b +0.00b 27.27s -2.83s 🚀

Cached Bundles

Bundle Size Difference Time Difference
dist/esm.c7dc1640.js 61.96kb +0.00b 1.05m +28.36s ⚠️
dist/workerHasher.e50d242f.js 1.72kb +0.00b 1.05m +28.36s ⚠️
dist/16.1969624f.js 1.08kb +0.00b 34.29s +2.94s ⚠️
dist/16.069344b7.js 905.00b +0.00b 34.29s +2.94s ⚠️
dist/simpleHasher.46d6f2e5.js 742.00b +0.00b 1.05m +28.36s ⚠️
dist/index.html 240.00b +0.00b 1.05m +35.55s ⚠️

Three.js ✅

Timings

Description Time Difference
Cold 6.15s -88.00ms
Cached 256.00ms -2.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

Click here to view a detailed benchmark overview.

@101arrowz
Copy link
Member

To avoid introducing a move function, why not intercept the 'finish' event (maybe via EventEmitter internals) and do what is currently in move before re-emitting 'finish'?

@mischnic mischnic changed the title More manual atomic createWriteStream Fix atomic createWriteStream Jun 10, 2022
@mischnic
Copy link
Member Author

This intercepting of the finish event was exactly what the previous broken implementation did.

I've now instead overridden the fs.close function used by fs.createWriteStream to move the file after closing the fd (and doing async operations is fine in this callback). So no await move() anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect js file extension with --no-scope-hoist
4 participants