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

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

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

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

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.

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

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.

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

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

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

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.

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

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

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

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

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

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?

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

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.

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

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