classification
Title: Raise errors from socket.close()
Type: enhancement Stage: resolved
Components: Extension Modules Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: 21069 Superseder:
Assigned To: Nosy List: haypo, martin.panter, python-dev, yselivanov
Priority: normal Keywords: patch

Created on 2016-04-01 11:16 by martin.panter, last changed 2017-07-04 14:46 by haypo. This issue is now closed.

Files
File name Uploaded Description Edit
socket.close.patch martin.panter, 2016-04-01 11:16 review
finalize-exception.patch martin.panter, 2016-04-04 01:44 Report finalization exceptions, unfinished review
socket.close.v2.patch martin.panter, 2016-04-04 02:03 review
Pull Requests
URL Status Linked Edit
PR 2566 merged haypo, 2017-07-04 14:31
Messages (20)
msg262735 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-04-01 11:16
While looking at a recent failure of test_fileno() in test_urllibnet, I discovered that socket.close() ignores the return value of the close() system call. It looks like it has always been this way: <https://docs.python.org/3/library/socket.html#socket.socket.fileno>.

On the other hand, both FileIO.close() and os.close() raise an exception if the underlying close() call fails. So I propose to make socket.close() also raise an exception for any failure indicated by the underlying close() call. The benefit is that a programming error causing EBADF would be more easily noticed.
msg262737 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2016-04-01 12:10
I like the idea :-)
msg262838 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-04-04 01:44
Victor suggested making these errors be reported by the finalize method (garbage collection). I started investigating this, but faced various test suite failures (test_io, test_curses), as well as many new unhandled exceptions printed out during the tests which do not actually trigger failures. Maybe this could become a worthwhile change, but it needs more investigation and wasn’t my original intention. Finalize-exception.patch is my patch so far.

There are many comments over the place that make me uneasy about this change, so I think it should be investigated separately. E.g. Modules/_io/iobase.c:

/* Silencing I/O errors is bad, but printing spurious tracebacks is
   equally as bad, and potentially more frequent (because of
   shutdown issues). */

Lib/_pyio.py:

# The try/except block is in case this is called at program
# exit time, when it's possible that globals have already been
# deleted, and then the close() call might fail.  Since
# there's nothing we can do about such failures and they annoy
# the end users, we suppress the traceback.
msg262839 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-04-04 02:03
Here is socket.close.v2.patch which hopefully fixes the test for Windows (still untested by me though)
msg263158 - (view) Author: Roundup Robot (python-dev) Date: 2016-04-11 01:20
New changeset bd665613ed67 by Martin Panter in branch 'default':
Issue #26685: Raise OSError if closing a socket fails
https://hg.python.org/cpython/rev/bd665613ed67
msg263161 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-04-11 02:51
I tweaked the test again for Windows, anticipating that it will raise ENOTSOCK rather than EBADF.
msg278946 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-10-18 22:50
I think this patch should be reverted.  It breaks backwards compatibility:

    import socket

    sock0 = socket.socket(socket.AF_INET, socket.SOCK_STREAM, 0)
    sock = socket.socket(
        socket.AF_INET, socket.SOCK_STREAM, 0, sock0.fileno())
    sock0.close()

    sock.close()


This code behaves differently on 3.5 vs 3.6.

In uvloop (https://github.com/magicstack/uvloop) I create Python sockets for libuv socket handles. Sometimes, those handles are closed by libuv, and I have no control over that. So right now, a lot of uvloop tests fail because socket.close() now can raise an error.

It also doesn't make any sense to raise it anyways.  socket.close() *must* be safe to call even when the socket is already closed.
msg278947 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-10-18 23:22
IOW, what is happening in uvloop:

1. A user has a Python socket object and passes it asyncio (run with uvloop) API.

2. uvloop extracts the FD of the socket, and passes it to low-level libuv.

3. At some point of time, libuv closes the FD.

4. When user tries to close the socket, it silently fails in 3.5, and raises an exception in 3.6.

One way to solve this would be to use os.dup() in uvloop (i.e. pass a duplicated FD to libuv).  But that would mean that programs that use uvloop will consume file descriptors (limited resource) faster than programs that use vanilla asyncio.

Currently, there's a documented way of creating a socket out of an existing FD (fourth argument to the socket's constructor).  This means that the FD might be controlled by someone.  At least in this case, sock.close() must not raise if it fails.  It's OK if the FD is already close, because the Python socket was only wrapping it in the first place.
msg278948 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-10-18 23:25
I think your code example is not very robust, because the “sock” object refers to a freed file descriptor, and could easily close an unrelated file:

$ python3.5 -q
>>> import socket
>>> sock0 = socket.socket()
>>> sock = socket.socket(fileno=sock0.fileno())
>>> sock0.close()
>>> f = open("/dev/null")  # Unrelated code/thread opens a file
>>> sock.close()  # Closes unrelated file descriptor!
>>> f.close()  # Error even in 3.5
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OSError: [Errno 9] Bad file descriptor

I am not familiar with your use case or library, but I would suggest that either:

1. Your code should call sock0.detach() rather than fileno(), so that sock0 no longer “owns” the file descriptor, or

2. libuv should not close file descriptors that it doesn’t own.

Calling socket.close() should be safe if the Python socket object is registered as closed, but IMO calling close(), or any other method, when the OS socket (file descriptor) has been released behind Python’s back is a programming error.
msg278949 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-10-18 23:38
If libuv closes the FD (step 3), won’t you get the same sort of problem if the uvloop user tries to do something else with the Python socket object, e.g. call getpeername()?

I see the fileno=... parameter for sockets as a parallel to the os.fdopen() function, which does raise exceptions from FileIO.close().

Maybe one option is to only trigger a DeprecationWarning, and raise a proper OSError in a future version.
msg278950 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-10-18 23:40
Another example: some asyncio (run with uvloop) code:
 
        conn, _ = lsock.accept()
        f = loop.create_task(
            loop.connect_accepted_socket(
                proto_factory, conn))
        # More code
        loop.run_forever()
        conn.close()

Suppose the above snippet of code is some real-world program.

Now, in Python 3.5 everything works as expected.  In Python 3.6, "conn.close()" will raise an exception.

Why: uvloop passes the FD of "conn" to libuv, which does its thing and closes the connection when it should be closed.

Now:

> 1. Your code should call sock0.detach() rather than fileno(), so that sock0 no longer “owns” the file descriptor, or

I can't modify "connect_accepted_socket" to call "detach" on "conn", as it would make "conn" unusable right after the call.  This option can't be implemented, as it would break the backwards compatibility.

> 2. libuv should not close file descriptors that it doesn’t own.

This is not just about uvloop/libuv.  It's about any Python program out there that will break in 3.6.  A lot of code extracts the FD out of socket objects and does something with it.  We can't just make socket.close() to raise in 3.6 -- that's not how backwards compatibility promise works.

Guido, what do you think about this issue?
msg278951 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-10-18 23:42
My proposal: ignore EBADF in socket.close().  It means that the socket is closed already.  It doesn't matter why.
msg278952 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2016-10-19 00:01
> My proposal: ignore EBADF in socket.close().  It means that the socket is closed already.  It doesn't matter why.

As Martin explained, getting EBAD means that your code smells. Your
code may close completely unrelated file descriptor... Say hello to
race conditions :-)
msg278953 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-10-19 00:11
Maybe we should fix asyncio.  Maybe if a user passes an existing socket object into loop.create_connection, it should duplicate the socket.
msg278983 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-10-19 15:54
After thinking about this problem for a while, I arrived to the conclusion that we need to fix asyncio.  Essentially, when a user passes a socket object to the event loop API like 'create_server', they give up the control of the socket to the loop.  The loop should detach the socket object and have a full control over it.
msg278984 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2016-10-19 16:01
"After thinking about this problem for a while, I arrived to the conclusion that we need to fix asyncio."

Ah, you became reasonable :-D

"Essentially, when a user passes a socket object to the event loop API like 'create_server', they give up the control of the socket to the loop.  The loop should detach the socket object and have a full control over it."

I don't know how asyncio should be modified exactly, but I agree that someone should change in asyncio. The question is more about how to reduce headache because of the backward compatibility and don't kill performances.

Would you mind to open an issue specific for asyncio?
msg278985 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-10-19 16:06
> Ah, you became reasonable :-D

Come on... :)

> Would you mind to open an issue specific for asyncio?

I'll open an issue on GH today to discuss with you/Guido/asyncio devs.
msg278991 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-10-19 19:49
>> Would you mind to open an issue specific for asyncio?

> I'll open an issue on GH today to discuss with you/Guido/asyncio devs.

Guido, Victor, please take a look: https://github.com/python/asyncio/pull/449
msg297656 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-07-04 14:20
New changeset 67e1478dba6efe60b8e1890192014b8b06dd6bd9 by Victor Stinner in branch 'master':
bpo-30319: socket.close() now ignores ECONNRESET (#2565)
https://github.com/python/cpython/commit/67e1478dba6efe60b8e1890192014b8b06dd6bd9
msg297658 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-07-04 14:46
New changeset 580cd5cd3603317d294a881628880a92ea221334 by Victor Stinner in branch '3.6':
bpo-30319: socket.close() now ignores ECONNRESET (#2565) (#2566)
https://github.com/python/cpython/commit/580cd5cd3603317d294a881628880a92ea221334
History
Date User Action Args
2017-07-04 14:46:12hayposetmessages: + msg297658
2017-07-04 14:31:42hayposetpull_requests: + pull_request2636
2017-07-04 14:20:08hayposetmessages: + msg297656
2016-10-19 19:49:25yselivanovsetmessages: + msg278991
2016-10-19 16:06:45yselivanovsetmessages: + msg278985
2016-10-19 16:01:26hayposetmessages: + msg278984
2016-10-19 15:54:39yselivanovsetstatus: open -> closed

messages: + msg278983
2016-10-19 00:11:43yselivanovsetmessages: + msg278953
2016-10-19 00:01:03hayposetmessages: + msg278952
2016-10-18 23:42:30yselivanovsetmessages: + msg278951
2016-10-18 23:40:24yselivanovsetmessages: + msg278950
2016-10-18 23:38:33martin.pantersetmessages: + msg278949
2016-10-18 23:25:42martin.pantersetmessages: + msg278948
2016-10-18 23:22:46yselivanovsetmessages: + msg278947
2016-10-18 22:50:01yselivanovsetstatus: closed -> open
nosy: + yselivanov
messages: + msg278946

2016-04-11 02:51:21martin.pantersetstatus: open -> closed
resolution: fixed
messages: + msg263161

stage: patch review -> resolved
2016-04-11 01:20:28python-devsetnosy: + python-dev
messages: + msg263158
2016-04-04 02:03:37martin.pantersetfiles: + socket.close.v2.patch

messages: + msg262839
2016-04-04 01:44:49martin.pantersetfiles: + finalize-exception.patch

messages: + msg262838
2016-04-01 12:10:23hayposetnosy: + haypo
messages: + msg262737
2016-04-01 11:17:49martin.pantersetdependencies: + test_fileno of test_urllibnet intermittently fails
2016-04-01 11:16:28martin.pantercreate