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

Replace select() syscall with Selectors module to fix filedescriptors out of range issue #7880

Closed
Eric-Arellano opened this issue Jun 12, 2019 · 1 comment · Fixed by #7882
Assignees

Comments

@Eric-Arellano
Copy link
Contributor

Eric-Arellano commented Jun 12, 2019

Problem

Twitter has 85 targets that fail to compile due to the message: filedescriptor out of range in select(), which sometimes traces to nailgun_executor.py and sometimes to util/socket.py:

readable, _, _ = select.select([ng_stdout], [], [], (-1 * remaining_time))

readable, _, _ = safe_select([self._socket], [], [], self._select_timeout)

We don't encounter this in Pants CI, because we don't try to compile as many targets as Twitter does (1500+ for these tests).

In the process of researching this, I found that many view the select() syscall as deprecated in favor of the more modern poll() and even more modern epoll(), devpoll(), and kqueue(). See https://beesbuzz.biz/code/5739-The-problem-with-select-vs-poll and https://stackoverflow.com/questions/970979/what-are-the-differences-between-poll-and-select.

Beyond generally being less desirable to use select(), it is very likely this is causing the filedescriptor out of range issue, per https://stackoverflow.com/a/41497735.

Proposed solution: Selectors library

Python 3.4 introduced the new selectors stdlib module via PEP 3156, built right on top of the low-level select stdlib module.

Beyond nice abstractions, the main benefit we get is the class DefaultSelector, which will choose the best syscall possible for that platform, e.g. kqueue on macOS / BSD and epoll() on Ubuntu. It will only revert to select() as a last resort, which will allow us to work around the filedescriptor issue, in addition to making the code more robust and easier to understand.

Handling Python 2 compatibility

This module was only introduced in Python 3.4, and was not backported. There does exist a 3rdparty backport selectors2 that we could use.

However, because we plan to drop Python 2 any day now—and the Twitter tests were passing when using Python 2 to run Pants—I propose that we keep the Python 2 code exactly how it is using select.select(), and only use selectors for Python 3 via an if PY3 check. In hopefully a week, we can kill off the Python 2 code branch.

@blorente
Copy link
Contributor

This is outstanding research, thanks!

I couldn't find the specific place where we set a high ulimit in Twitter code, but I don't think it matters because it's fairly safe to assume we do, and this change is good regardless.

I'm definitely on board with using the selectors stdlib module. For the PY2 compat, I lean in favour of keeping the old code behind a PY2 flag, I think it will make it easier to remove once we no longer support PY2, and there's very little that can go wrong.

@Eric-Arellano Eric-Arellano self-assigned this Jun 12, 2019
Eric-Arellano added a commit that referenced this issue Jun 13, 2019
### Problem
Twitter was running into the error `filedescriptor out of range in select()` when compiling a large number of targets, per #7880.

In the process, we found that it is more efficient and robust to use the more modern syscalls of `epoll()`, `devpoll()`, and `kqueue()` than `select()`. Python 3.4 introduced the Selectors module to choose the best syscall available for us.

This will close #7880.

### Solution
For Python 3 code, use the new selectors library. Python 2 code stays the same as before, since the library does not exist for it and we are soon going to drop Python 2.
Eric-Arellano added a commit that referenced this issue Jun 13, 2019
### Problem
Twitter was running into the error `filedescriptor out of range in select()` when compiling a large number of targets, per #7880.

In the process, we found that it is more efficient and robust to use the more modern syscalls of `epoll()`, `devpoll()`, and `kqueue()` than `select()`. Python 3.4 introduced the Selectors module to choose the best syscall available for us.

This will close #7880.

### Solution
For Python 3 code, use the new selectors library. Python 2 code stays the same as before, since the library does not exist for it and we are soon going to drop Python 2.
cattibrie pushed a commit to cattibrie/pants that referenced this issue Jun 19, 2019
…ld#7882)

### Problem
Twitter was running into the error `filedescriptor out of range in select()` when compiling a large number of targets, per pantsbuild#7880.

In the process, we found that it is more efficient and robust to use the more modern syscalls of `epoll()`, `devpoll()`, and `kqueue()` than `select()`. Python 3.4 introduced the Selectors module to choose the best syscall available for us.

This will close pantsbuild#7880.

### Solution
For Python 3 code, use the new selectors library. Python 2 code stays the same as before, since the library does not exist for it and we are soon going to drop Python 2.
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 a pull request may close this issue.

2 participants