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

Use asynchronous operations everywhere to avoid blocking the main thread #948

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dcodeIO
Copy link

@dcodeIO dcodeIO commented Nov 22, 2024

This PR removes all blocking operations (child_process.execSync and fs.readSync) from the library and replaces them with asynchronous equivalents while keeping the public API intact.

The diff is intentionally boring, focusing solely on this change and minor related cleanup to minimize the risk of introducing accidental breakage.

Fixes #873. Keep it up!

Comment on lines -1139 to -1149
function promisifySave(nodeStyleFunction) {
return function () {
const args = Array.prototype.slice.call(arguments);
return new Promise(function (resolve) {
args.push(function (err, data) {
resolve(data);
});
nodeStyleFunction.apply(null, args);
});
};
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This utility function had exactly one usage, where now a .catch(() => '') is utilized in its place.

@lukechilds
Copy link

lukechilds commented Nov 23, 2024

Just fyi @sebhildebrandt systeminformation causes major performance issues and event loop deadlocks when used in server applications. This PR fixes it.

https://nodejs.org/en/learn/asynchronous-work/dont-block-the-event-loop

We're running this fork at @getumbrel with great results but we'd love to come back to upstream once this is merged 🙏

@Spartan-Hex-Shadow
Copy link

@sebhildebrandt would you be able to include this in the next release?

@sebhildebrandt
Copy link
Owner

@Spartan-Hex-Shadow, @dcodeIO , @lukechilds ... the next major release (6.0) is a complete rewrite (TS) where we do not have any sync functions. This should then be solved.

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.

Awaiting promises returned by systeminformation can still block running thread
4 participants