Fix hang up when a lot of parallel operation request the file system

[garthenweb]

↪️ Pull Request

tl;dr: When a lot of parallel operations request libuv thread pool, the bundler is not able to handle the response from the promise and the process stops working.

I am currently migrating a larger project form Webpack to Parcel. When running the build, at some point the build just stops, also CPU is idle. When the process is restarted (multiple times) by hand, at some point in time the build succeeds, because the cache is already warmed up.

I tried to run the build with different options, in development and production mode, with and without (experimental) scope hoisting and also with different node versions (10 and 11) using the src and lib version. It's always the same result, even if not 100% deterministic (it stops at different files). I experienced the issue on Mac OSX and Windows (using WSL) in version v1.10.x and v1.11.0.

No error is provided, even when the log level is set to verbose.

Tracing the issue within the Parcel source code guided me to findPackage function which did not receive a response from readPackage when the hang up happens. I also found out that it did not matter whether the promise resolved or rejected, it was just never fulfilled. readPackage calls fs.readFile from @parcel/fs under the hood. Changing the @parcel/fs functions to the native (experimental) promise implementation didn't change anything, but switching all functions to the sync equivalent (like fs.readFileSync) solved it. But it was slower...

Searching the issue history of Parcel I found issues like #1331 with a solution: #901. Using the PARCEL_MAX_CONCURRENT_CALLS environment actually fixes the issue as well for me, but it was as slow as the fs.readFileSync solution (~110 seconds). Besides that, it took me some time to actually find it, I would love to have a solution that works out of the box.

Accessing the file system in NodeJS makes use of the libuv thread pool which by default uses up to schedule 4 parallel threads. Increasing the default UV_THREADPOOL_SIZE solved the issue for me without any performance downsides (~95 seconds). NodeJS allows to use an environment variable to set the thread pool size, but if called before the first API calls libuv we can set it within the code, therefore I added it to the entry points I am aware of (please let me know if there might be more).

I am not an expert of the libuv or how the thread pool size exactly works, I also don't understand to 100% why the promise will not resolve in this scenario. But as far as I can see there is not a big downside on increasing the thread pool size in case no CPU intensive operations are done within it. See http://docs.libuv.org/en/v1.x/threadpool.html. As far as I can see, Parcel mostly uses it for accessing the file system which should not be CPU intensive. Therefore 16 seems to be a good default for Parcel (I didn't saw any performance difference when changing the value, but everything above 6 worked for my use case).

💻 Examples

Sadly I cannot provide an example because the project I am working on is private. Maybe others that had the same issue can prove that it's working.

🚨 Test instructions

I guess running Parcel on a bigger code base will force this issue at some point in time. Important to mention is, that I always run Parcel from scratch (without cache).

✔️ PR Todo

I hope that is all :) I am happy to receive feedback!

[devongovett]

Great analysis! Thanks for taking the time to write this up. 🥇

I am fine with changing the default for the CLI, but I'm not sure if we should overwrite it when Parcel is embedded in other people's programs. Perhaps they will have different work loads in addition to parcel that might be adversely affected by this? Not sure.

Also, should we do the same thing in the worker processes (i.e. worker.js)? Since those are different copies of node, they might not get the same environment variable set when they are spawned.

[garthenweb]

Great analysis! Thanks for taking the time to write this up. 🥇

Welcome, thank you for putting so much time into this awesome tool!

I am fine with changing the default for the CLI, but I'm not sure if we should overwrite it when Parcel is embedded in other people's programs. Perhaps they will have different work loads in addition to parcel that might be adversely affected by this? Not sure.

Didn't thought about it, but makes perfectly sense to me. If we change it for the bundler, it would probably be a breaking change because it affects the overall NodeJS runtime.
Therefore I also propose to move the line to ./bin/cli and remove it from the src folder at all.
Further it would makes sense to me to add it to the documentation so others are aware about this. What do you think is a good place to have this described? Maybe in the API section? I can create a PR for this as well as soon as this is merged.

Also, should we do the same thing in the worker processes (i.e. worker.js)? Since those are different copies of node, they might not get the same environment variable set when they are spawned.

I just checked by using console.log that it is already passed into the worker.js if set before in the cli, therefore I think it should not be required to set it there as well. I guess otherwise it would also not have been working before.

To keep the history clean I would now amend the discussed changes to the existing commit and force push. Are you good with that or do you prefere to reflect the discussion in the history?

[garthenweb]

@devongovett As discussed I updated the implementation to only affect the cli and rebased the branch. Please let me know whether you have more opinions about the implementation :)

[garthenweb]

@DeMoorJasper @devongovett Is anything missing or unclear to get this merged?