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

close-all-files uses too much resources #581

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thomasgoirand
Copy link

Since Systemd 256~rc3-3, maxfd could be really big, and therefore, could take too much resources. This patch attemps to limit this.

Since Systemd 256~rc3-3, maxfd could be *really* big,
and therefore, could take too much resources. This patch
attemps to limit this.
@CendioOssman
Copy link
Member

Thanks for your suggested fix!

I'm a bit cautious about just picking an arbitrary limit, though. Perhaps it's better to move away from this brute force method and instead enumerate open file descriptors by looking in /proc/self/fd instead?

@thomasgoirand
Copy link
Author

Hi.
Indeed, I would have like to do it better as well, but didn't find out how to enumerate the open FDs in a nice way. There's also the os.closeall() function that may be used too, maybe it's possible to just call it with the lowest (3) and highest value possible in the system, and let it deal with it? I honestly don't have enough time to investigate. I'm only a package maintainer in Debian, busy packaging all of OpenStack, Ceph, OpenVSwitch, RabbitMQ and so on, so there's only a limited amount of time I can invest in each bug. The current patch I used just does the job: the package isn't stuck in a loop doing tests. So I'll let you investigate a better solution that I'll adopt as soon as you release it. Thanks for your understanding.

@CendioOssman
Copy link
Member

I assume you mean os.closerange()? It should have similar issues. Might be fast enough, since it's likely written in C instead. But might not.

For the '/proc/self/fd' approach, it's not terribly complex:

diff --git a/websockify/websockifyserver.py b/websockify/websockifyserver.py
index 74f9f53..c1631c7 100644
--- a/websockify/websockifyserver.py
+++ b/websockify/websockifyserver.py
@@ -17,8 +17,7 @@ import multiprocessing
 from http.server import SimpleHTTPRequestHandler
 
 # Degraded functionality if these imports are missing
-for mod, msg in [('ssl', 'TLS/SSL/wss is disabled'),
-                 ('resource', 'daemonizing is disabled')]:
+for mod, msg in [('ssl', 'TLS/SSL/wss is disabled')]:
     try:
         globals()[mod] = __import__(mod)
     except ImportError:
@@ -383,8 +382,6 @@ class WebSockifyServer():
         # Sanity checks
         if not ssl and self.ssl_only:
             raise Exception("No 'ssl' module and SSL-only specified")
-        if self.daemon and not resource:
-            raise Exception("Module 'resource' required to daemonize")
 
         # Show configuration
         self.msg("WebSocket server settings:")
@@ -519,9 +516,9 @@ class WebSockifyServer():
         signal.signal(signal.SIGINT, signal.SIG_IGN)
 
         # Close open files
-        maxfd = resource.getrlimit(resource.RLIMIT_NOFILE)[1]
-        if maxfd == resource.RLIM_INFINITY: maxfd = 256
-        for fd in reversed(range(maxfd)):
+        fds = os.listdir('/proc/self/fd')
+        for fd in fds:
+            fd = int(fd)
             try:
                 if fd not in keepfd:
                     os.close(fd)

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.

None yet

2 participants