[issue32038] Add API to intercept socket.close()

[Yury Selivanov]

Yury Selivanov  added the comment:

Closing this one. Using private socket.socket APIs works fine for 
uvloop/asyncio.

--
resolution:  -> rejected
stage:  -> resolved
status: open -> closed
type:  -> enhancement

___
Python tracker 

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

[issue32038] Add API to intercept socket.close()

[Yury Selivanov]

Yury Selivanov  added the comment:

> Note that a guard on socket objects can only solve part of the problem: in 
> the case where i've seen this bug, it was with raw file descriptors from 
> libcurl, and there's nothing python can do about that because there are no 
> (python) socket objects.

Well, in this case 'dup' is the only option.

--

___
Python tracker 

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

[issue32038] Add API to intercept socket.close()

[Ben Darnell]

Ben Darnell  added the comment:

Note that a guard on socket objects can only solve part of the problem: in the 
case where i've seen this bug, it was with raw file descriptors from libcurl, 
and there's nothing python can do about that because there are no (python) 
socket objects.

--

___
Python tracker 

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

[issue32038] Add API to intercept socket.close()

[Yury Selivanov]

Yury Selivanov  added the comment:

> OK, then maybe socket objects should get a dup() method that just
increments the ref counter, and that would be the public API you're looking
for?

"dup()" returns a new socket object, but here we only want to protect the 
original one.

Maybe just 'sock.inc_io_ref()' and 'sock.dec_io_ref()'?  OTOH using the current 
private socket API in asyncio and uvloop doesn't really scare me as long as it 
works, so keeping the status quo is also OK.

--

___
Python tracker 

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

[issue32038] Add API to intercept socket.close()

[Guido van Rossum]

Guido van Rossum  added the comment:

OK, then maybe socket objects should get a dup() method that just
increments the ref counter, and that would be the public API you're looking
for?

--

___
Python tracker 

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

[issue32038] Add API to intercept socket.close()

[Yury Selivanov]

Yury Selivanov  added the comment:

> Perhaps you can just dup() the socket? That's what the ref counter is for 
> IIRC.

I already dup them for loop.create_server(sock=sock) and 
loop.create_connection(sock=sock) cases.  Duping for sock_recv & friends will 
add too much overhead (they are relatively short operations).  Another argument 
against duping is that the number of open FDs is a limited OS resource.

So far Victor's idea of using '_io_refs' sounds like a perfect fit for both 
asyncio and uvloop.  After all, asyncio use case is very similar to 
`socket.makefile()` -- keep the socket object alive while another API relies on 
it.

--

___
Python tracker 

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

[issue32038] Add API to intercept socket.close()

[STINNER Victor]

STINNER Victor  added the comment:

> Perhaps you can just dup() the socket? That's what the ref counter is for 
> IIRC.

Sure. That's another option.

But for asyncio with a lot of concurrent users, I'm not sure that it's ok to 
create a temporary file descriptor, since there is a risk of hitting the 
maximum number of open file descriptors :-(

Depending on the platform and the event loop implementation, the limit may be 
low.

--

___
Python tracker 

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

[issue32038] Add API to intercept socket.close()

[Guido van Rossum]

Guido van Rossum  added the comment:

Perhaps you can just dup() the socket? That's what the ref counter is for
IIRC.

--

___
Python tracker 

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

[issue32038] Add API to intercept socket.close()

[Yury Selivanov]

Yury Selivanov  added the comment:

> Hmm... _decref_socketios() is not really a public API.  I think it would be 
> nice to have an official way to deal with this, and a socket close callback 
> sounds reasonable to me.


We can just make it public (after some renames) :)  My first idea was to add a 
refcounting API to sockets, I just didn't know it's already there (I assumed 
that socketmodule.c implements the actual 'socket' object, and never even 
looked in socket.py module).

I'll experiment with this stuff in asyncio/uvloop and report back to this issue.

--

___
Python tracker 

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

[issue32038] Add API to intercept socket.close()

[STINNER Victor]

STINNER Victor  added the comment:

> Hmm... _decref_socketios() is not really a public API.  I think it would be 
> nice to have an official way to deal with this, and a socket close callback 
> sounds reasonable to me.

I dislike the idea of an event when a socket is closed. It doesn't prevent a 
crash if you are using the socket with the GIL released.

I prefer to "hold" the socket while using it.

--

___
Python tracker 

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

[issue32038] Add API to intercept socket.close()

[Antoine Pitrou]

Antoine Pitrou  added the comment:

Hmm... _decref_socketios() is not really a public API.  I think it would be 
nice to have an official way to deal with this, and a socket close callback 
sounds reasonable to me.

--

___
Python tracker 

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

[issue32038] Add API to intercept socket.close()

[Yury Selivanov]

Yury Selivanov  added the comment:

> Oh, sock._decref_socketios() must be uesd here to really close the socket if 
> it was closed in the meanwhile:

There's also 'sock._closed' attribute that sock.close() and sock. 
_decref_socketios() interact with.. Alright, let me play with this in uvloop.  
So far it looks like we don't need any new APIs :)

--

___
Python tracker 

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

[issue32038] Add API to intercept socket.close()

[Yury Selivanov]

Change by Yury Selivanov :


--
Removed message: https://bugs.python.org/msg306374

___
Python tracker 

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

[issue32038] Add API to intercept socket.close()

[Yury Selivanov]

Yury Selivanov  added the comment:

> Oh, sock._decref_socketios() must be uesd here to really close the socket if 
> it was closed in the meanwhile:

For it to work correctly, socket objects should be initialized with 
"self._io_refs = 1".

--

___
Python tracker 

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

[issue32038] Add API to intercept socket.close()

[STINNER Victor]

STINNER Victor  added the comment:

>From my example: "finally: sock._io_refs -= 1"

Oh, sock._decref_socketios() must be uesd here to really close the socket if it 
was closed in the meanwhile:

def _decref_socketios(self):
if self._io_refs > 0:
self._io_refs -= 1
if self._closed:
self.close()

--

___
Python tracker 

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

[issue32038] Add API to intercept socket.close()

[Yury Selivanov]

Yury Selivanov  added the comment:

> The socket.socket (Python type) has a private _io_refs counter. close() does 
> nothing until _io_refs reachs zero.

I didn't know about this.  Looks like I can use '_io_refs' to fix some problems 
in uvloop, thanks Victor!

The only problem with '_io_refs' and `dont_close_socket` approach is that the 
original intent to close the socket is "lost".  Consider this example:

  fut = loop.sock_recv(sock)
  sock.close()
  await fut

Ideally, I'd expect `sock` to be closed right after `await fut` is completed.  
One way to make that possible is to initialize socket object with 
'self._io_refs = 1', not 0.  Whenever it goes to 0, the socket gets closed.  
This is a slight change to the existing machinery, but it would make 
sock.close() more predictable.

--

___
Python tracker 

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

[issue32038] Add API to intercept socket.close()

[STINNER Victor]

STINNER Victor  added the comment:

The socket.socket (Python type) has a private _io_refs counter. close() does 
nothing until _io_refs reachs zero.

Maybe we can develop an API to temporary increase the _io_refs counter to 
prevent a real close?

Pseudo-code:

@contextlib.contextmanager
def dont_close_socket(sock):
assert not sock._closed
sock._io_refs += 1
try:
yield sock
finally:
sock._io_refs -= 1

It may be a context manager or a new object. It would probably be better to put 
in the socket.socke type, since it uses private attributes.

--
nosy: +haypo

___
Python tracker 

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

[issue32038] Add API to intercept socket.close()

[Yury Selivanov]

Yury Selivanov  added the comment:

> It's worse than a resource leak - the same file descriptor number could be 
> reused for a different file/socket, and then depending on the selector in 
> use, you could see the data from a completely different connection. 

I actually debugged a bug like this in asyncio code once.  Took me quite a bit 
of time to figure it out.

> I did see a bug like this years ago (in libcurl), although it's not a common 
> problem. I'd use the proposed hook if it existed, but it seems like an 
> intrusive solution to a rare issue.

I don't think the proposed solution is too intrusive.  If we don't like the 
"set a callback to intercept all socket.close()" idea, we can change it to: 
"add socket.add_close_callback() method to the socket object."

--

___
Python tracker 

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

[issue32038] Add API to intercept socket.close()

[Ben Darnell]

Ben Darnell  added the comment:

It's worse than a resource leak - the same file descriptor number could be 
reused for a different file/socket, and then depending on the selector in use, 
you could see the data from a completely different connection. 

I did see a bug like this years ago (in libcurl), although it's not a common 
problem. I'd use the proposed hook if it existed, but it seems like an 
intrusive solution to a rare issue.

--

___
Python tracker 

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

[issue32038] Add API to intercept socket.close()

[Antoine Pitrou]

Antoine Pitrou  added the comment:

Ok, that sounds reasonable to me.  Also cc'ing Ben Darnell (maintainer of 
Tornado) in case he wants to chime in.

--
nosy: +Ben.Darnell

___
Python tracker 

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

[issue32038] Add API to intercept socket.close()

[Yury Selivanov]

Yury Selivanov  added the comment:

> Why not instead add `loop.sock_close()`?

It wouldn't help if the program calls `socket.close()` somewhere.  This can 
happen easily in big code bases.

Ideally (at least I think so) we need to provide a guarantee that the socket 
object can't be closed at all (or be closed in a controlled manner) if it's 
being operated by asyncio/event-loop.

--

___
Python tracker 

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

[issue32038] Add API to intercept socket.close()

[Antoine Pitrou]

Antoine Pitrou  added the comment:

Why not instead add `loop.sock_close()`?

--

___
Python tracker 

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

[issue32038] Add API to intercept socket.close()

[Yury Selivanov]

New submission from Yury Selivanov :

asyncio has a few APIs that accept a Python socket object: loop.sock_recv(), 
loop.sock_accept() etc.

asyncio (and event loops such as uvloop) expect that it controls the socket for 
the duration of the call.  However, it cannot prevent the socket object from 
being closing *while* it is working with it:

loop = asyncio.get_event_loop()
sock = socket.socket()
sock.connect(('google.com', 80))
sock.setblocking(0)
f = loop.sock_recv(sock, 100)
sock.close()
await f

The above snippet makes asyncio to forever try to read from 'sock' (resource 
leak).  uvloop simply segfaults (at least on Linux) because libuv assumes that 
sockets can't be closed while it's working with them.

Same problem exists when a user calls `loop.create_server(sock=sock)` and later 
manually calls `sock.close()`.

My proposal is to add a new API: `socket.register_close(callback)`.  `callback` 
will be called whenever a socket object is about to be closed.

This will enable asyncio and uvloop to detect when sockets they are working 
with are about to be closed and to either raise an error, log a debug line, 
or/and to stop polling the socket.

See also: https://github.com/MagicStack/uvloop/issues/100

--
components: IO, asyncio
messages: 306298
nosy: asvetlov, gvanrossum, pitrou, yselivanov
priority: normal
severity: normal
status: open
title: Add API to intercept socket.close()
versions: Python 3.7

___
Python tracker 

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