Fix unclosed socket ResourceWarnings in werkzeug.serving

[TomiBelan]

This PR fixes #2421 and addresses the remaining warnings not fixed by PR #2498. It refactors the code from #2321.

What caused the issue

There are three cases which emit a ResourceWarning, involving three different sockets:

1. werkzeug.serving.BaseWSGIServer.__init__ calls super().__init__(...) and assigns self.socket = socket.fromfd(...). But self.socket already exists at the time. If you add print(self.socket) before the assignment, you'll see it's not None. It is an unbound socket unconditionally opened by TCPServer.__init__. The unclosed socket warning actually refers to the old socket, not the one returned by socket.fromfd(). Werkzeug's assignment causes the original socket to be destructed/finalized.

   This explanation answers @davidism's confusion in https://github.com/pallets/werkzeug/issues/2421#issuecomment-1208624459. It works fine for http.server because it never reassigns self.socket.

2. The reloader parent process creates a socket s with prepare_socket() and never closes it. ➜ This part was fixed in #2498 by adding s.detach().

3. The reloader parent process calls srv = make_server(...), which calls socket.fromfd() to duplicate the socket. In fact, srv is almost unused, except for this log_startup() call. The parent calls run_with_reloader(srv.serve_forever, ...), but main_func is only used in the child process, and completely ignored in the parent. Hence, srv.serve_forever is never called, hence, srv.server_close is never called, hence, srv.socket stays open.

How this PR fixes it (in probably too much detail)

Case 1 is fixed by just closing the socket first. (If we could make changes to Python itself, it would be slightly more elegant to add a "socket" argument to TCPServer itself, and not open the other one at all. But it's not a big deal.)

Case 3 could be fixed by calling srv.socket.detach() in the reloader parent process. But it didn't feel very elegant. I don't like that the parent calls prepare_socket() and then duplicates the socket unnecessarily. And it would be nicer to close the socket properly instead of detach().

I refactored run_simple to stop using prepare_socket(). The socket is always created with make_server(). The reloader parent process explicitly calls socket_close() at the end. This also reduces code duplication (e.g. the os.unlink call was duplicated).

To help you review this PR, here is a line by line breakdown of prepare_socket() and how it matches make_server():

• select_address_family(hostname, port): also called in BaseWSGIServer
• get_sockaddr(hostname, port, address_family): also called in BaseWSGIServer
• socket.socket(address_family, socket.SOCK_STREAM): also called in BaseWSGIServer.__init__ -> TCPServer.__init__
• s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1): called in TCPServer.server_bind because I added allow_reuse_address = True
• s.set_inheritable(True): moved to run_simple (changed: now it's only done in the reloader parent process)
• server_address = t.cast(str, server_address), os.unlink(server_address): also called in BaseWSGIServer (changed: now it happens earlier - before the socket.socket call)
• s.bind(server_address): also called in BaseWSGIServer.__init__ -> TCPServer.server_bind
• except OSError as e: moved to BaseWSGIServer (although I'm a bit unsure if printing to stderr and calling sys.exit inside the constructor is a good idea; if you prefer, I could add a class _BindError(Exception) and catch it in run_simple)
• s.listen(LISTEN_QUEUE): also called in BaseWSGIServer.__init__ -> TCPServer.server_activate (note that we already had request_queue_size = LISTEN_QUEUE)

Demo

$ python --version
Python 3.10.4
$ cat demo.py
import sys
from werkzeug.serving import run_simple

def app(environ, start_response):
    start_response('200 OK', [('Hello', 'world')])
    return [b'body\n']

run_simple(
    hostname="localhost",
    port=int(sys.argv[1]),
    application=app,
    use_reloader={'1': True, '0': False}[sys.argv[2]],
)
$ git switch main
$ PYTHONWARNINGS=default python demo.py 5000 1
/home/user/werkzeug/src/werkzeug/serving.py:718: ResourceWarning: unclosed <socket.socket fd=4, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=0, laddr=('0.0.0.0', 0)>
  self.socket = socket.fromfd(fd, address_family, socket.SOCK_STREAM)
ResourceWarning: Enable tracemalloc to get the object allocation traceback
WARNING: This is a development server. Do not use it in a production deployment. Use a production WSGI server instead.
 * Running on http://localhost:5000
Press CTRL+C to quit
 * Restarting with watchdog (inotify)
/home/user/werkzeug/src/werkzeug/serving.py:718: ResourceWarning: unclosed <socket.socket fd=4, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=0, laddr=('0.0.0.0', 0)>
  self.socket = socket.fromfd(fd, address_family, socket.SOCK_STREAM)
ResourceWarning: Enable tracemalloc to get the object allocation traceback
127.0.0.1 - - [19/Sep/2022 23:17:03] "GET / HTTP/1.1" 200 -
 * Detected change in '/home/user/werkzeug/demo.py', reloading
 * Detected change in '/home/user/werkzeug/demo.py', reloading
 * Restarting with watchdog (inotify)
/home/user/werkzeug/src/werkzeug/serving.py:718: ResourceWarning: unclosed <socket.socket fd=4, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=0, laddr=('0.0.0.0', 0)>
  self.socket = socket.fromfd(fd, address_family, socket.SOCK_STREAM)
ResourceWarning: Enable tracemalloc to get the object allocation traceback
127.0.0.1 - - [19/Sep/2022 23:17:12] "GET / HTTP/1.1" 200 -
^C
/home/user/werkzeug/demo.py:8: ResourceWarning: unclosed <socket.socket fd=5, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=0, laddr=('127.0.0.1', 5000)>
  run_simple(
ResourceWarning: Enable tracemalloc to get the object allocation traceback
$ PYTHONWARNINGS=default python demo.py 8000 1
Address already in use
Port 8000 is in use by another program. Either identify and stop that program, or start the server with a different port.
sys:1: ResourceWarning: unclosed <socket.socket fd=3, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=0, laddr=('0.0.0.0', 0)>
ResourceWarning: Enable tracemalloc to get the object allocation traceback
$ git switch warnings
$ PYTHONWARNINGS=default python demo.py 5000 1
WARNING: This is a development server. Do not use it in a production deployment. Use a production WSGI server instead.
Press CTRL+C to quit
 * Running on http://localhost:5000
 * Restarting with watchdog (inotify)
127.0.0.1 - - [19/Sep/2022 23:17:39] "GET / HTTP/1.1" 200 -
 * Detected change in '/home/user/werkzeug/demo.py', reloading
 * Detected change in '/home/user/werkzeug/demo.py', reloading
 * Restarting with watchdog (inotify)
127.0.0.1 - - [19/Sep/2022 23:17:41] "GET / HTTP/1.1" 200 -
^C
$ PYTHONWARNINGS=default python demo.py 8000 1
Address already in use
Port 8000 is in use by another program. Either identify and stop that program, or start the server with a different port.

I ran curl -v http://localhost:5000/ and touch demo.py at appropriate times. I also tested it with the reloader disabled. I'd be grateful if someone could try it on Windows, just in case.

Checklist:

• [ ] Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
• [ ] Add or update relevant docs, in the docs folder and in code.
• [x] Add an entry in CHANGES.rst summarizing the change and linking to the issue.
• [ ] Add .. versionchanged:: entries in any relevant code docs.
• [x] Run pre-commit hooks and fix any issues.
• [ ] Run pytest and tox, no tests failed.

By the way: Thank you for your hard work. I'm a big fan.

[davidism]

Thanks for the great explanation.

[TomiBelan]

Force-pushed to fix typing issues.

[TomiBelan]

Oh, right, should I add a warnings.warn(..., DeprecationWarning) to prepare_socket? Or remove it right away? (it's technically public but it's not in the html docs)

[TomiBelan]

Note, if you're planning to release this in 2.2.x then you might also want to adjust the .. deprecated:: 2.3 Will be removed in Werkzeug 2.4. in prepare_socket.

A DeprecationWarning could help too, but I'm not familiar with that.

[davidism]

Reworked to keep the changes closer to the original code. Removed prepare_socket, it's new and pretty internal so I don't think it's worth the deprecation.

Confirmed that this doesn't show any warnings on Python 3.11rc2 with python -X dev.

[TomiBelan]

Sounds good. Thanks for merging! 🎉