The PEP 442 helped to make object finalization safer, but it's just an API, it's not widely used in the Python core yet. io.FileIO has a nice implementation, but socket.socket and os.scandir don't. I noticed this while working on the issue #26567 which indirectly resurected a destroyed socket (in test_socket). As I workaround, I reverted my change on socket destructor, but I'm interested to enhance socket destructor to be able to use the new tracemalloc feature of the warnings module.

Attached patch adds a finalizer to _socket.socket() and use PyErr_ResourceWarning() to log the traceback where the socket was created in the warning logger (if tracemalloc is enabled, see issue #26567).

It's unclear to me if it's needed to call the following code in sock_dealloc(): + if (PyObject_CallFinalizerFromDealloc((PyObject *)s) < 0) + return; It looks like this code is not needed, since sock_finalize() is called before sock_dealloc().

sock_finalize() is only called explicitly if there is a reference cycle. This is why sock_dealloc() has to call it.

2016-03-19 11:05 GMT+01:00 Antoine Pitrou <report@bugs.python.org>: > sock_finalize() is only called explicitly if there is a reference cycle. This is why sock_dealloc() has to call it. I'm fine with keeping sock_dealloc() to call sock_finalize(), but I would like to understand. Example: --- import socket s=socket.socket() s=None --- With this code, sock_finalize() is called before sock_dealloc(): #0 sock_finalize (s=0x7ffff0730c28) at /home/haypo/prog/python/default/Modules/socketmodule.c:4172 #1 0x00000000004d8f59 in PyObject_CallFinalizer (self=<socket at remote 0x7ffff0730c28>) at Objects/object.c:294 #2 0x00000000004d8fcd in PyObject_CallFinalizerFromDealloc (self=<socket at remote 0x7ffff0730c28>) at Objects/object.c:311 #3 0x00000000004f2c8f in subtype_dealloc (self=<socket at remote 0x7ffff0730c28>) at Objects/typeobject.c:1154 #4 0x00000000004dc8ae in _Py_Dealloc (op=<socket at remote 0x7ffff0730c28>) at Objects/object.c:1783

@Antoine: Since you wrote the PEP 442, would you reviewing sock_finalize.patch? I'm only interested to modify Python 3.6, the bug was only trigerred when I changed the code to log a warning. Before, the socket object was not passed to the warning logger so it worked fine.

Le 19/03/2016 14:07, STINNER Victor a écrit : > > Example: > --- > import socket > s=socket.socket() > s=None > --- > > With this code, sock_finalize() is called before sock_dealloc(): > > #0 sock_finalize (s=0x7ffff0730c28) at > /home/haypo/prog/python/default/Modules/socketmodule.c:4172 > #1 0x00000000004d8f59 in PyObject_CallFinalizer (self=<socket at > remote 0x7ffff0730c28>) at Objects/object.c:294 > #2 0x00000000004d8fcd in PyObject_CallFinalizerFromDealloc > (self=<socket at remote 0x7ffff0730c28>) at Objects/object.c:311 > #3 0x00000000004f2c8f in subtype_dealloc (self=<socket at remote > 0x7ffff0730c28>) at Objects/typeobject.c:1154 Ah, that's probably because socket.socket is a Python subclass. What happens if you use _socket.socket directly instead?

Antoine Pitrou added the comment: > Ah, that's probably because socket.socket is a Python subclass. > What happens if you use _socket.socket directly instead? Oh, I forgot this sublte implementation detail, _socket.socket base class vs socket.socket sublcass. Example with _socket: --- import _socket s=_socket.socket() s=None --- Ok, in this case sock_finalize() is called by sock_dealloc(). --- #0 sock_finalize (s=0x7ffff7eaad60) at /home/haypo/prog/python/default/Modules/socketmodule.c:4172 #1 0x00000000004d8f59 in PyObject_CallFinalizer (self=<_socket.socket at remote 0x7ffff7eaad60>) at Objects/object.c:294 #2 0x00000000004d8fcd in PyObject_CallFinalizerFromDealloc (self=<_socket.socket at remote 0x7ffff7eaad60>) at Objects/object.c:311 #3 0x00007ffff04e326a in sock_dealloc (s=0x7ffff7eaad60) at /home/haypo/prog/python/default/Modules/socketmodule.c:4192 #4 0x00000000004dc8ae in _Py_Dealloc (op=<_socket.socket at remote 0x7ffff7eaad60>) at Objects/object.c:1783 ---

Now that there is a safe finalizer, I wonder if it should release the GIL, as in sock_close().

> Now that there is a safe finalizer, I wonder if it should release the GIL, as in sock_close(). Ah yes, good idea. I updated my patch. I also changed the change to begin by setting the sock_fd attribute to -1, before closing the socket (since the GIL is now released, the order matters).

On the review, Antoine wrote: "You should put this line earlier, as outputting a warning can release the GIL..." I disagree. It's a deliberate choice to keep the socket open while logging the ResourceWarning. Example: --- import socket import warnings def show(msg): s = msg.source #s.close() if s.fileno() >= 0: print("socket open") else: print("socket closed") try: name = s.getsockname() except Exception as exc: name = str(exc) print("getsockname(): %r" % (name,)) warnings._showwarnmsg = show s = socket.socket() s = None --- Output with sock_finalize-2.patch: --- socket open getsockname(): ('0.0.0.0', 0) --- If you uncomment the s.close() (or set sock_fd to -1 in the C code): --- socket closed getsockname(): '[Errno 9] Bad file descriptor' --- IMHO it's ok to give access to socket methods in the warning logger.

Oh, I see. Perhaps add a comment then?

> Oh, I see. Perhaps add a comment then? Sure, done. Does it look better now?

Yes, it looks good to me.

New changeset 46329eec5515 by Victor Stinner in branch 'default': Add socket finalizer https://hg.python.org/cpython/rev/46329eec5515

Thanks for the review. I pushed my change.

New changeset a97d317dec85 by Victor Stinner in branch 'default': Fix test_ssl.test_refcycle() https://hg.python.org/cpython/rev/a97d317dec85