novnc / websockify

Websockify is a WebSocket to TCP proxy/bridge. This allows a browser to connect to any application/server/service.
GNU Lesser General Public License v3.0
3.95k stars 781 forks source link

close-all-files uses too much resources #581

Open thomasgoirand opened 4 months ago

thomasgoirand commented 4 months ago

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

CendioOssman commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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)