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: ensure that the correct number of CPU cores is obtained within t… #570

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

Conversation

isbasex
Copy link

@isbasex isbasex commented Jun 21, 2023

PR Description

Fixes Incorrect CPU Core Count within Docker Container

This PR contains a:

  • bugfix

Motivation / Use-Case:

This PR addresses a bug occurring within Docker containers due to an incorrect assumption about the available CPU cores. In the original code, os.cpus().length was used to obtain the number of cores. This works fine in most scenarios, but within a Docker container, this would return the number of cores available to the host system, not the limited number allocated to the container itself. This discrepancy leads to creating an excessive number of concurrent threads, which can ultimately result in an out-of-memory (OOM) error.

This PR corrects this issue by ensuring that the number of cores reported matches the actual number allocated to the Docker container, preventing the creation of too many threads and subsequent OOM errors.

Breaking Changes:

This PR does not introduce any breaking changes.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 21, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@@ -60,6 +60,7 @@
},
"dependencies": {
"@jridgewell/trace-mapping": "^0.3.18",
"get_cpus_length": "^1.0.3",
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it is a good idea to read system files https://github.com/xiaoxiaojx/get_cpus_length/blob/master/index.js#L21

Copy link
Member

Choose a reason for hiding this comment

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

Just wonder, is a problem happens for all docker containers and for all versions?

Copy link
Author

Choose a reason for hiding this comment

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

In our testing, all Linux containers have this issue.

Currently, no better method has been found. If this method is not used, would you consider not setting the concurrency quantity completely according to the CPU cores by default?

For example: Math.min(4, os.cpus().length)

I think this fix is very important for robustness, especially for large projects.

Copy link
Member

Choose a reason for hiding this comment

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

I think this fix is very important for robustness, especially for large projects.

I agree with you, just want to find the better solution. How we can detect is it a docker container? Maybe we can disable/improve logic without reading system files.

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.

2 participants