msg262014 - (view) |
Author: STINNER Victor (vstinner) *  |
Date: 2016-03-19 01:31 |
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.
|
msg262031 - (view) |
Author: STINNER Victor (vstinner) *  |
Date: 2016-03-19 09:38 |
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).
|
msg262032 - (view) |
Author: STINNER Victor (vstinner) *  |
Date: 2016-03-19 09:39 |
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().
|
msg262035 - (view) |
Author: Antoine Pitrou (pitrou) *  |
Date: 2016-03-19 10:05 |
sock_finalize() is only called explicitly if there is a reference cycle. This is why sock_dealloc() has to call it.
|
msg262045 - (view) |
Author: STINNER Victor (vstinner) *  |
Date: 2016-03-19 13:07 |
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
|
msg262046 - (view) |
Author: STINNER Victor (vstinner) *  |
Date: 2016-03-19 13:14 |
@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.
|
msg262047 - (view) |
Author: Antoine Pitrou (pitrou) *  |
Date: 2016-03-19 13:15 |
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?
|
msg262048 - (view) |
Author: STINNER Victor (vstinner) *  |
Date: 2016-03-19 13:18 |
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
---
|
msg262116 - (view) |
Author: Antoine Pitrou (pitrou) *  |
Date: 2016-03-21 10:40 |
Now that there is a safe finalizer, I wonder if it should release the GIL, as in sock_close().
|
msg262118 - (view) |
Author: STINNER Victor (vstinner) *  |
Date: 2016-03-21 10:53 |
> 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).
|
msg262124 - (view) |
Author: STINNER Victor (vstinner) *  |
Date: 2016-03-21 13:26 |
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.
|
msg262126 - (view) |
Author: Antoine Pitrou (pitrou) *  |
Date: 2016-03-21 13:41 |
Oh, I see. Perhaps add a comment then?
|
msg262128 - (view) |
Author: STINNER Victor (vstinner) *  |
Date: 2016-03-21 14:05 |
> Oh, I see. Perhaps add a comment then?
Sure, done.
Does it look better now?
|
msg262130 - (view) |
Author: Antoine Pitrou (pitrou) *  |
Date: 2016-03-21 14:46 |
Yes, it looks good to me.
|
msg262133 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2016-03-21 15:38 |
New changeset 46329eec5515 by Victor Stinner in branch 'default':
Add socket finalizer
https://hg.python.org/cpython/rev/46329eec5515
|
msg262135 - (view) |
Author: STINNER Victor (vstinner) *  |
Date: 2016-03-21 16:02 |
Thanks for the review. I pushed my change.
|
msg262139 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2016-03-21 16:30 |
New changeset a97d317dec85 by Victor Stinner in branch 'default':
Fix test_ssl.test_refcycle()
https://hg.python.org/cpython/rev/a97d317dec85
|
|
Date |
User |
Action |
Args |
2022-04-11 14:58:28 | admin | set | github: 70777 |
2016-03-21 16:30:25 | python-dev | set | messages:
+ msg262139 |
2016-03-21 16:02:31 | vstinner | set | status: open -> closed resolution: fixed messages:
+ msg262135
|
2016-03-21 15:38:58 | python-dev | set | nosy:
+ python-dev messages:
+ msg262133
|
2016-03-21 14:46:46 | pitrou | set | messages:
+ msg262130 |
2016-03-21 14:05:55 | vstinner | set | files:
+ sock_finalize-3.patch
messages:
+ msg262128 |
2016-03-21 13:41:28 | pitrou | set | messages:
+ msg262126 |
2016-03-21 13:26:51 | vstinner | set | messages:
+ msg262124 |
2016-03-21 12:53:42 | serhiy.storchaka | set | nosy:
+ serhiy.storchaka
|
2016-03-21 10:53:20 | vstinner | set | files:
+ sock_finalize-2.patch
messages:
+ msg262118 |
2016-03-21 10:40:34 | pitrou | set | messages:
+ msg262116 |
2016-03-19 13:18:30 | vstinner | set | messages:
+ msg262048 |
2016-03-19 13:15:09 | pitrou | set | messages:
+ msg262047 |
2016-03-19 13:14:01 | vstinner | set | messages:
+ msg262046 |
2016-03-19 13:07:01 | vstinner | set | messages:
+ msg262045 |
2016-03-19 10:05:05 | pitrou | set | messages:
+ msg262035 |
2016-03-19 09:39:29 | vstinner | set | messages:
+ msg262032 |
2016-03-19 09:38:39 | vstinner | set | files:
+ sock_finalize.patch keywords:
+ patch messages:
+ msg262031
|
2016-03-19 01:31:15 | vstinner | create | |