[issue26590] socket destructor: implement finalizer

2016-03-21 Thread Roundup Robot 

Roundup Robot added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26590] socket destructor: implement finalizer

2016-03-21 Thread STINNER Victor 

STINNER Victor added the comment:

Thanks for the review. I pushed my change.

--
resolution:  -> fixed
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26590] socket destructor: implement finalizer

2016-03-21 Thread Roundup Robot 

Roundup Robot added the comment:

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

--
nosy: +python-dev

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26590] socket destructor: implement finalizer

2016-03-21 Thread Antoine Pitrou 

Antoine Pitrou added the comment:

Yes, it looks good to me.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26590] socket destructor: implement finalizer

2016-03-21 Thread STINNER Victor 

STINNER Victor added the comment:

> Oh, I see. Perhaps add a comment then?

Sure, done.

Does it look better now?

--
Added file: http://bugs.python.org/file42233/sock_finalize-3.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26590] socket destructor: implement finalizer

2016-03-21 Thread Antoine Pitrou 

Antoine Pitrou added the comment:

Oh, I see. Perhaps add a comment then?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26590] socket destructor: implement finalizer

2016-03-21 Thread STINNER Victor 

STINNER Victor added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26590] socket destructor: implement finalizer

2016-03-21 Thread Serhiy Storchaka 

Changes by Serhiy Storchaka :


--
nosy: +serhiy.storchaka

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26590] socket destructor: implement finalizer

2016-03-21 Thread STINNER Victor 

STINNER Victor added the comment:

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

--
Added file: http://bugs.python.org/file42230/sock_finalize-2.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26590] socket destructor: implement finalizer

2016-03-21 Thread Antoine Pitrou 

Antoine Pitrou added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26590] socket destructor: implement finalizer

2016-03-19 Thread Antoine Pitrou 

Antoine Pitrou added the comment:

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=0x70730c28) at
> /home/haypo/prog/python/default/Modules/socketmodule.c:4172
> #1  0x004d8f59 in PyObject_CallFinalizer (self= remote 0x70730c28>) at Objects/object.c:294
> #2  0x004d8fcd in PyObject_CallFinalizerFromDealloc
> (self=) at Objects/object.c:311
> #3  0x004f2c8f in subtype_dealloc (self= 0x70730c28>) 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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26590] socket destructor: implement finalizer

2016-03-19 Thread STINNER Victor 

STINNER Victor added the comment:

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=0x77eaad60) at
/home/haypo/prog/python/default/Modules/socketmodule.c:4172
#1  0x004d8f59 in PyObject_CallFinalizer (self=<_socket.socket
at remote 0x77eaad60>) at Objects/object.c:294
#2  0x004d8fcd in PyObject_CallFinalizerFromDealloc
(self=<_socket.socket at remote 0x77eaad60>) at
Objects/object.c:311
#3  0x704e326a in sock_dealloc (s=0x77eaad60) at
/home/haypo/prog/python/default/Modules/socketmodule.c:4192
#4  0x004dc8ae in _Py_Dealloc (op=<_socket.socket at remote
0x77eaad60>) at Objects/object.c:1783
---

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26590] socket destructor: implement finalizer

2016-03-19 Thread STINNER Victor 

STINNER Victor added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26590] socket destructor: implement finalizer

2016-03-19 Thread STINNER Victor 

STINNER Victor added the comment:

2016-03-19 11:05 GMT+01:00 Antoine Pitrou :
> 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=0x70730c28) at
/home/haypo/prog/python/default/Modules/socketmodule.c:4172
#1  0x004d8f59 in PyObject_CallFinalizer (self=) at Objects/object.c:294
#2  0x004d8fcd in PyObject_CallFinalizerFromDealloc
(self=) at Objects/object.c:311
#3  0x004f2c8f in subtype_dealloc (self=) at Objects/typeobject.c:1154
#4  0x004dc8ae in _Py_Dealloc (op=) at Objects/object.c:1783

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26590] socket destructor: implement finalizer

2016-03-19 Thread Antoine Pitrou 

Antoine Pitrou added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26590] socket destructor: implement finalizer

2016-03-19 Thread STINNER Victor 

STINNER Victor added the comment:

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

--
keywords: +patch
Added file: http://bugs.python.org/file42215/sock_finalize.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26590] socket destructor: implement finalizer

2016-03-19 Thread STINNER Victor 

STINNER Victor added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26590] socket destructor: implement finalizer

2016-03-18 Thread STINNER Victor 

New submission from STINNER Victor:

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.

--
messages: 262014
nosy: haypo, pitrou
priority: normal
severity: normal
status: open
title: socket destructor: implement finalizer
type: enhancement
versions: Python 3.6

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com