socket destructor: implement finalizer

[vstinner]

BPO	26590
Nosy	@pitrou, @vstinner, @serhiy-storchaka
Files	sock_finalize.patch sock_finalize-2.patch sock_finalize-3.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2016-03-21.16:02:31.131>
created_at = <Date 2016-03-19.01:31:15.269>
labels = ['type-feature']
title = 'socket destructor: implement finalizer'
updated_at = <Date 2016-03-21.16:30:25.182>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2016-03-21.16:30:25.182>
actor = 'python-dev'
assignee = 'none'
closed = True
closed_date = <Date 2016-03-21.16:02:31.131>
closer = 'vstinner'
components = []
creation = <Date 2016-03-19.01:31:15.269>
creator = 'vstinner'
dependencies = []
files = ['42215', '42230', '42233']
hgrepos = []
issue_num = 26590
keywords = ['patch']
message_count = 17.0
messages = ['262014', '262031', '262032', '262035', '262045', '262046', '262047', '262048', '262116', '262118', '262124', '262126', '262128', '262130', '262133', '262135', '262139']
nosy_count = 4.0
nosy_names = ['pitrou', 'vstinner', 'python-dev', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue26590'
versions = ['Python 3.6']

[vstinner]

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 bpo-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.

[vstinner]

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 bpo-26567).

[vstinner]

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().

[pitrou]

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

[vstinner]

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

[vstinner]

@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.

[pitrou]

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?

[vstinner]

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
---

[pitrou]

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

[vstinner]

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).

[vstinner]

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.

[pitrou]

Oh, I see. Perhaps add a comment then?

[vstinner]

Oh, I see. Perhaps add a comment then?

Sure, done.

Does it look better now?

[pitrou]

Yes, it looks good to me.