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

stage2: ask for more file descriptors #6921

Merged

Conversation

xackus
Copy link
Contributor

@xackus xackus commented Nov 1, 2020

Follow up to #6906.

@andrewrk
Copy link
Member

andrewrk commented Nov 1, 2020

I believe you also get to delete this code now too :)

zig/src/stage1/os.cpp

Lines 981 to 1003 in 445d808

#if defined(ZIG_OS_POSIX)
// Raise the open file descriptor limit.
// Code lifted from node.js
struct rlimit lim;
if (getrlimit(RLIMIT_NOFILE, &lim) == 0 && lim.rlim_cur != lim.rlim_max) {
// Do a binary search for the limit.
rlim_t min = lim.rlim_cur;
rlim_t max = 1 << 20;
// But if there's a defined upper bound, don't search, just set it.
if (lim.rlim_max != RLIM_INFINITY) {
min = lim.rlim_max;
max = lim.rlim_max;
}
do {
lim.rlim_cur = min + (max - min) / 2;
if (setrlimit(RLIMIT_NOFILE, &lim)) {
max = lim.rlim_cur;
} else {
min = lim.rlim_cur;
}
} while (min + 1 < max);
}
#endif

@xackus xackus force-pushed the gimmeMoreOfThoseSweetSweetFileDescriptors branch from c74d890 to 7703f4c Compare November 1, 2020 22:32
@andrewrk andrewrk added enhancement Solving this issue will likely involve adding new logic or components to the codebase. frontend Tokenization, parsing, AstGen, Sema, and Liveness. labels Nov 1, 2020
@xackus
Copy link
Contributor Author

xackus commented Nov 2, 2020

I don't have a mac, so I would appreciate help with the EINVAL failure from someone who does. cc @kubkon
I made my linux machine go through the binary search path and it worked fine.

On Darwin, according to the man pages for setrlimit(), when adjusting
max number of open fds, the reported hard max by getrlimit() is only
theoretical, while the actual maximum, set in the kernel, is hardcoded
in the header file. Therefore, the reported max has to be adjusted
as `min(OPEN_MAX, lim.max)`.

Signed-off-by: Jakub Konka <kubkon@jakubkonka.com>
@kubkon
Copy link
Member

kubkon commented Nov 2, 2020

@xackus 8dda64f should fix it on Darwin :-) The issue on Darwin is that you can't trust getrlimit() for NOFILE. According to the man pages:

setrlimit() now returns with errno set to EINVAL in places that historically succeeded. It no longer accepts "rlim_cur = RLIM_INFINITY" for RLIM_NOFILE. Use "rlim_cur = min(OPEN_MAX, rlim_max)".

@xackus
Copy link
Contributor Author

xackus commented Nov 2, 2020

@kubkon Thank you, I missed the "COMPATIBILITY" section of the man page.

@xackus
Copy link
Contributor Author

xackus commented Nov 3, 2020

This looks like another argument against EINVAL => unreachable. See #6389, #6925.

@kubkon
Copy link
Member

kubkon commented Nov 3, 2020

This looks like another argument against EINVAL => unreachable. See #6389, #6925.

I guess, in this case, we could already map EINVAL to some proper error value. However, we just need to ensure we make it impossible to specify an incorrect resource value. For instance, on Darwin EINVAL can be returned in two instances. From the man pages:

 [EINVAL]           resource is invalid.
 [EINVAL]           The specified limit is invalid (e.g., RLIM_INFINITY or lower than rlim_cur).

Since we constrain the input resource to resource_limit enum, we should be OK mapping EINVAL exclusively to invalid limit error condition.

EDIT: proposing that for another PR, not necessarily this one.

@kubkon
Copy link
Member

kubkon commented Nov 3, 2020

The CI failure is unrelated to this PR, so proceeding with the merge.

@kubkon kubkon merged commit 5060497 into ziglang:master Nov 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Solving this issue will likely involve adding new logic or components to the codebase. frontend Tokenization, parsing, AstGen, Sema, and Liveness.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants