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

select.select() does not support file descriptors > 1023 #78

Open
gbanasiak opened this issue Dec 13, 2024 · 6 comments · May be fixed by #81
Open

select.select() does not support file descriptors > 1023 #78

gbanasiak opened this issue Dec 13, 2024 · 6 comments · May be fixed by #81

Comments

@gbanasiak
Copy link
Contributor

Thespian version : recent snapshot (4a7b522)

OS: Ubuntu 22.04, Linux kernel 6.5.0-1025-gcp

Problem:

When using multiprocTCPBase with high horizontal scaling (hundreds of actors), I've run into the following error which breaks Thespian:

2024-12-13 13:11:00.591990 p23331 I    bad internal file descriptor!
2024-12-13 13:11:00.592023 p23331 I    wrecv 1049 is bad

After adding extra logs:

diff --git a/thespian/system/transport/TCPTransport.py b/thespian/system/transport/TCPTransport.py
index bbff82e..1cd7081 100644
--- a/thespian/system/transport/TCPTransport.py
+++ b/thespian/system/transport/TCPTransport.py
@@ -1182,20 +1182,20 @@ class TCPTransport(asyncTransportBase, wakeupTransportBase):
                             thesplog('bad internal file descriptor!')
                             try:
                                 _ = select.select([self.socket.fileno()], [], [], 0)
-                            except Exception:
-                                thesplog('listen %s is bad', self.socket.fileno)
+                            except Exception as e:
+                                thesplog('listen %s is bad: %s', self.socket.fileno, e)
                                 rerr.append(self.socket.fileno)
                             for each in wrecv:
                                 try:
                                     _ = select.select([each], [], [], 0)
-                                except Exception:
-                                    thesplog('wrecv %s is bad', each)
+                                except Exception as e:
+                                    thesplog('wrecv %s is bad: %s', each, e)
                                     rerr.append(each)
                             for each in wsend:
                                 try:
                                     select.select([each], [], [], 0)
-                                except Exception:
-                                    thesplog('wsend %s is bad', each)
+                                except Exception as e:
+                                    thesplog('wsend %s is bad: %s', each, e)
                                     rerr.append(each)
                         else:
                             self._watches = [W for W in self._watches if W not in bad]

I can see the following:

2024-12-13 13:37:30.963284 p61218 I    bad internal file descriptor!
2024-12-13 13:37:30.963335 p61218 I    wrecv 1049 is bad: filedescriptor out of range in select()

The process which encounters this definitely has limits properly raised:

$ cat /proc/61218/limits
Limit                     Soft Limit           Hard Limit           Units
Max cpu time              unlimited            unlimited            seconds
Max file size             unlimited            unlimited            bytes
Max data size             unlimited            unlimited            bytes
Max stack size            8388608              unlimited            bytes
Max core file size        0                    0                    bytes
Max resident set          unlimited            unlimited            bytes
Max processes             unlimited            unlimited            processes
Max open files            500000               500000               files          <---- HERE
Max locked memory         unlimited            unlimited            bytes
Max address space         unlimited            unlimited            bytes
Max file locks            unlimited            unlimited            locks
Max pending signals       2063717              2063717              signals
Max msgqueue size         819200               819200               bytes
Max nice priority         0                    0
Max realtime priority     0                    0
Max realtime timeout      unlimited            unlimited            us

I think we are running into select.select() limitation caused by FD_SETSIZE constant value which is documented in select() system call.

POSIX allows an implementation to define an upper limit, advertised via the constant FD_SETSIZE, on the range of file descriptors that can be specified in a file descriptor set. The Linux kernel imposes no fixed limit, but the glibc implementation makes fd_set a fixed-size type, with FD_SETSIZE defined as 1024, and the FD_*() macros operating according to that limit. To monitor file descriptors greater than 1023, use poll(2) or epoll(7) instead.

Can we replace select.select() with select.poll() or selectors? Any preferences? I am assuming select.epoll() is not an option as it is only supported by Linux. I am happy to work on the PR.

This is a serious scaling limitation for rally.

@gbanasiak gbanasiak changed the title select.select() does not support file descriptors > 1024 select.select() does not support file descriptors > 1023 Dec 13, 2024
@kquick
Copy link
Member

kquick commented Dec 13, 2024

Switching to poll sounds like a reasonable solution. It looks like switching to poll could probably be done mostly with isolation to this particular function; longer term the poll interface might support some additional efficiencies in registering which descriptors to watch, but I'd suggest the initial PR should be more directly applied and then any efficiency improvements (if you wanted to do them) should probably be done as a second phase since it would be more invasive to the overall TCP support.

@gbanasiak
Copy link
Contributor Author

Sounds good. I'll get back once I have something functional.

@gbanasiak
Copy link
Contributor Author

I've realized select.poll() is not supported on MacOS, so that's not an option for Rally. Now I think selectors are probably the way to go. selectors.DefaultSelector() resolves to optimal underlying method depending on the platform. My first simplest attempt seems to work, but I'm not happy with the performance, it's slightly worse than select.select(). I think I need to optimize by not creating and closing a new selector instance on every loop but calculate deltas and register/unregister file descriptors accordingly, still within this single function. But it does lift the 1024 limit on both Linux and MacOS (not on Windows which still uses select.select() under the hood I think). I haven't tested error handling at all, so this needs some checks too.

However, selectors came in Python 3.4. Would that be acceptable for Thespian?

@gbanasiak
Copy link
Contributor Author

gbanasiak commented Dec 16, 2024

I'm comparing multiplexing performance using new example script.

% python examples/socketstress.py 1200
creating dispatcher...
initiating workers start...
waiting 3 seconds...
run!
run completed in 1.4279401249950752 seconds

For select.select() to selectors comparison 300 workers should be enough.

@gbanasiak
Copy link
Contributor Author

I've tried reducing selector instance creations as in c4350ca but it did not bring performance improvements. It also caused a few tests to fail, and I couldn't figure out why. I've raised #81 with the simplest approach hoping selectors are acceptable. This passed pytest on MacOS and Linux, haven't tested Windows.

@gbanasiak
Copy link
Contributor Author

I've tested #81 on Windows 10 with Python 3.12. Simple use (examples/hellogoodbye.py, examples/socketstress.py) seems to work fine but there are > 170 failed tests, many of them unrelated (e.g. multiprocUDPBase). I see a similar, yet not identical number of failed tests with recent main. Now I'm wondering whether Windows is considered fully supported in Thespian.

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