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

Add hard fileno #366

Merged
merged 5 commits into from
Nov 8, 2024
Merged

Add hard fileno #366

merged 5 commits into from
Nov 8, 2024

Conversation

Ongy
Copy link
Contributor

@Ongy Ongy commented Oct 9, 2024

This allows wstunnel to use fds>1024.
Otherwise heavy use can easily run out of file descriptors on connection
attempts.
While there will still be a limit, it is significantly higher (~500
times on my system) which provides enough headroom for connections to be
torn down and fds to be closed.

This allows wstunnel to use fds>1024.
Otherwise heavy use can easily run out of file descriptors on connection
attempts.
While there will still be a limit, it is significantly higher (~500
times on my system) which provides enough headroom for connections to be
torn down and fds to be closed.
@erebe
Copy link
Owner

erebe commented Oct 10, 2024

Hello,

Thank you for the PR, it is appreciated.
But ultimately, I would prefer to let this setting outside of wstunnel. For me it is up to the system to configure the file descriptor limit. Either with systemd, ulimit, netconfig,...

In addition, this crate only works on linux/macos, so it should be gated by a conditonnal compilation cfg.

Or maybe make it the default without any flag for those os.

@Ongy
Copy link
Contributor Author

Ongy commented Oct 22, 2024

Hi, sorry for the late reply

While I fully agree that the hard limit should be set by the environment, the soft limit is mostly a historical issue with the 1024 FDs support by select(2). IMO it makes sense to up it in process when the application acknowledges that it can handle it.
Might even make sense without the flag 🤔

Yea, gating to the OS makes sense. I'll have to figure out the syntax if that's the only issue.

@erebe
Copy link
Owner

erebe commented Oct 25, 2024

I think we can make it the default without any flags and merge the PR.
I checked the crate, and it does already nothing if it is a platform it does not support.

So calling everytime fdlimit::raise_fd_limit() is fine for me as long as we dont exit if the call fails, and just log a warning.

@Ongy
Copy link
Contributor Author

Ongy commented Nov 7, 2024

Done.

Do you have a preference about squashing?

@erebe
Copy link
Owner

erebe commented Nov 8, 2024

That's perfect, thank you.
I am going to squash into 1 commit

@erebe erebe merged commit 0504395 into erebe:main Nov 8, 2024
13 checks passed
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