python / cpython

The Python programming language
https://www.python.org
Other
61.88k stars 29.76k forks source link

Garbage collector is not respecting port exhaustion in the runtime #104688

Closed ManiMozaffar closed 1 year ago

ManiMozaffar commented 1 year ago

Bug report

What happens is that when you're using asyncio.get_event_loop(), it opens a connection in windows, with a port. then later, if you delete that object and it has 0 ref on the memory, it doesn't get collected up by gc. I don't understand why it opened a connection in the first place, so I can't consider if that's the issue, but what I can consider as issue is when I delete the object, and I garbage collect it, it doesn't get collected, it only does when script is paused. so I believe GC is not considering port exhaustion in the runtime, and it should do in this case to avoid bug and port exhaustion.

import asyncio
import psutil
import os
import gc

def check_connections():
    """Check count of ESTABLISHED connections."""
    return len([
        conn for conn in psutil.net_connections()
        if conn.status == 'ESTABLISHED' and conn.pid == os.getpid()
    ])

loop = asyncio.get_event_loop()
print(check_connections()) #2
loop = None # or del loop
gc.collect()
print(check_connections()) #2

Your environment

ManiMozaffar commented 1 year ago
class PipeServer(object):
    """Class representing a pipe server.

    This is much like a bound, listening socket.
    """
    def __init__(self, address):
        self._address = address
        self._free_instances = weakref.WeakSet()
        # initialize the pipe attribute before calling _server_pipe_handle()
        # because this function can raise an exception and the destructor calls
        # the close() method
        self._pipe = None
        self._accept_pipe_future = None
        self._pipe = self._server_pipe_handle(True)

    def _get_unconnected_pipe(self):
        # Create new instance and return previous one.  This ensures
        # that (until the server is closed) there is always at least
        # one pipe handle for address.  Therefore if a client attempt
        # to connect it will not fail with FileNotFoundError.
        tmp, self._pipe = self._pipe, self._server_pipe_handle(False)
        return tmp

    def _server_pipe_handle(self, first):
        # Return a wrapper for a new pipe handle.
        if self.closed():
            return None
        flags = _winapi.PIPE_ACCESS_DUPLEX | _winapi.FILE_FLAG_OVERLAPPED
        if first:
            flags |= _winapi.FILE_FLAG_FIRST_PIPE_INSTANCE
        h = _winapi.CreateNamedPipe(
            self._address, flags,
            _winapi.PIPE_TYPE_MESSAGE | _winapi.PIPE_READMODE_MESSAGE |
            _winapi.PIPE_WAIT,
            _winapi.PIPE_UNLIMITED_INSTANCES,
            windows_utils.BUFSIZE, windows_utils.BUFSIZE,
            _winapi.NMPWAIT_WAIT_FOREVER, _winapi.NULL)
        pipe = windows_utils.PipeHandle(h)
        self._free_instances.add(pipe)
        return pipe

    def closed(self):
        return (self._address is None)

    def close(self):
        if self._accept_pipe_future is not None:
            self._accept_pipe_future.cancel()
            self._accept_pipe_future = None
        # Close all instances which have not been connected to by a client.
        if self._address is not None:
            for pipe in self._free_instances:
                pipe.close()
            self._pipe = None
            self._address = None
            self._free_instances.clear()

    __del__ = close

This code has problem, the problem is that GC never collects PipeServer's object, I have no idea why, even if all ports by system has been taken. so on my sample test code, you can actually see that gc didn't do what it was supposed to do.

It only does it, when you exit runtime. and it's too late, it's useless. the last line of code, del = close is useless line, because when process is finished on windows, all connections bounded to it will terminate as well. so it doesn't matter if GC actually clean it up or not after runtime, it only matters during runtime.

eryksun commented 1 year ago

get_event_loop() is supported by a thread-local reference to the event loop. Call set_event_loop(None) to clear the internal reference, but first you should explicitly close() the loop instead of relying on the garbage collector to finalize the object. Details like this are handled for you by the high-level asyncio.run() function.

Here's the behavior of a new Proactor event loop that hasn't been set as the current thread's event loop.

>>> import asyncio, gc, psutil
>>> p = psutil.Process()
>>> len(p.connections())
0
>>> loop = asyncio.new_event_loop()
>>> loop
<ProactorEventLoop running=False closed=False debug=False>
>>> len(p.connections()) # socketpair from BaseProactorEventLoop._make_self_pipe
2
>>> loop.close() # calls BaseProactorEventLoop._close_self_pipe
>>> len(p.connections())
0
>>> loop = asyncio.new_event_loop()
>>> del loop
>>> len(p.connections()) # the loop wasn't explicitly closed
2
>>> gc.collect() # finalizing closes the sockets
28
>>> len(p.connections())
0

OTOH, if the loop is set as the current thread's event loop, then it remains referenced even if all external references to it have been deleted.

>>> loop = asyncio.new_event_loop()
>>> asyncio.set_event_loop(loop)
>>> len(p.connections())
2
>>> del loop
>>> gc.collect()
0
>>> len(p.connections())
2
>>> asyncio.set_event_loop(None)
>>> len(p.connections())
2
>>> gc.collect()
0
>>> len(p.connections())
0
ManiMozaffar commented 1 year ago

OTOH, if the loop is set as the current thread's event loop, then it remains referenced even if all external references to it have been deleted.

>>> loop = asyncio.new_event_loop()
>>> asyncio.set_event_loop(loop)
>>> len(p.connections())
2
>>> del loop
>>> gc.collect()
0
>>> len(p.connections())
2
>>> asyncio.set_event_loop(None)
>>> len(p.connections())
2
>>> gc.collect()
0
>>> len(p.connections())
0

This code, still shows as 2 to me. so I'm afraid, even setting current thread's event loop to None will not fix this issue, only what it does is using .close(). Make sure you're running code on windows, not linux.

I've 4 questions;

  1. Wouldn't it be better if we could use async context manager to handle such situation? because in this situation, if somehow the event loop is not closed, or we forget to close, then it remain as a port exhaustion leak.
  2. you said first I should explicitly close() the loop instead of relying on the garbage collector to finalize the object, ok that's understable, but what is the code __del__ doing exactly if gc is not doing its job?
  3. Why do we even open a socket port at first place? I didn't understand the reason.
  4. can't it access the current process connection, and reuse the same port? Normally it does that, but as I've seen in seleniumbase library, it somehow doesn't. because each event loop was created in different threads, and each time a new port was occupied by this code.
import asyncio
import psutil
import os
import gc

def check_connections():
    """Check count of ESTABLISHED connections."""
    return len([
        conn for conn in psutil.net_connections()
        if conn.status == 'ESTABLISHED' and conn.pid == os.getpid()
    ])

loop = asyncio.new_event_loop()
asyncio.set_event_loop(loop)
asyncio.set_event_loop(None)
del loop
gc.collect()
print(check_connections()) #2
gvanrossum commented 1 year ago

This report seems based on some misunderstanding about how event loops work. Please find a user forum to discuss this issue further.