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

Raise Soft File Descriptor Limit Up To The Hard Limit #10650

Merged
merged 17 commits into from
May 20, 2022

Conversation

nisdas
Copy link
Member

@nisdas nisdas commented May 6, 2022

What type of PR is this?

Feature Addition

What does this PR do? Why is it needed?

Which issues(s) does this PR fix?

N.A

Other notes for review

@nisdas nisdas requested a review from a team as a code owner May 6, 2022 10:21
@nisdas nisdas requested review from kasey, rauljordan and symbolpunk May 6, 2022 10:21
kasey
kasey previously approved these changes May 6, 2022
package fdlimits

import (
"github.com/ethereum/go-ethereum/common/fdlimit"
Copy link
Contributor

Choose a reason for hiding this comment

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

I see no downside to doing this by default, so 👍. I looked at the go-ethereum lib because I was curious why we were adding this dependency on them. It looks like the benefit here is that they have the major os-specific variants set up with appropriate build tags, so that we can have this simple Raise(Maximum()) code (including eg Windows where the raise is a no-op). seems fine.


func TestSetMaxFdLimits(t *testing.T) {
var limit syscall.Rlimit
assert.NoError(t, syscall.Getrlimit(syscall.RLIMIT_NOFILE, &limit))
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this break tests on Windows?

Copy link
Member Author

Choose a reason for hiding this comment

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

This wouldnt work on windows admittedly. I can add in a check to only run this on linux/macos.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the test could just be to call the method to Raise to the Max, then request Current and compare it to Max? That way the tests stick to the go-ethereum methods (assuming we implicitly trust that these methods are doing the correct thing on each platform).

Copy link
Contributor

@kasey kasey left a comment

Choose a reason for hiding this comment

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

LGTM!

@prylabs-bulldozer prylabs-bulldozer bot merged commit f28b47b into develop May 20, 2022
@delete-merged-branch delete-merged-branch bot deleted the addLimits branch May 20, 2022 08:12
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.

3 participants