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: increase buffer size #3600

Merged
merged 1 commit into from
Aug 17, 2022
Merged

fix: increase buffer size #3600

merged 1 commit into from
Aug 17, 2022

Conversation

YairZ101
Copy link
Contributor

@YairZ101 YairZ101 commented Aug 17, 2022

What does this PR do?

The PR fixes the problem where in some cases when scanning a directory with a high number of files, the scan failed due to an ENOBUFS error.
The fix is rather simple, it simply increases the max buffer.

@YairZ101 YairZ101 requested a review from ofekatr August 17, 2022 14:57
@YairZ101 YairZ101 requested a review from a team as a code owner August 17, 2022 14:57
@github-actions
Copy link
Contributor

github-actions bot commented Aug 17, 2022

Warnings
⚠️

You've modified files in src/ directory, but haven't updated anything in test folder. Is there something that could be tested?

Generated by 🚫 dangerJS against 8079fe3

@@ -53,6 +53,7 @@ function scanWithConfig(
const process = childProcess.spawnSync(policyEnginePath, args, {
encoding: 'utf-8',
stdio: 'pipe',
maxBuffer: 1024 * 1024 * 10,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know what value was originally being used?
Do we know how bigger the buffer's cap just became?
Why was this value chosen in particular?

@@ -53,6 +53,7 @@ function scanWithConfig(
const process = childProcess.spawnSync(policyEnginePath, args, {
encoding: 'utf-8',
stdio: 'pipe',
maxBuffer: 1024 * 1024 * 10,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default value is 1024 * 1024, if we see in the future that multiplying it by 10 is not enough we can increase it further.

@YairZ101 YairZ101 force-pushed the fix/increase-buffer-size branch from 06aadca to 8079fe3 Compare August 17, 2022 15:16
@YairZ101 YairZ101 enabled auto-merge August 17, 2022 15:16
@YairZ101 YairZ101 merged commit d1b7560 into master Aug 17, 2022
@YairZ101 YairZ101 deleted the fix/increase-buffer-size branch August 17, 2022 15:28
@ofekatr
Copy link
Contributor

ofekatr commented Aug 17, 2022

@YairZ101 Can you please share this solution with #cli for visibility?

@YairZ101
Copy link
Contributor Author

@ofekatr why do you think we should post this in #cli? this PR doesn't affect the CLI as a whole.
Do you have any concerns?

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