Change socket.close() to ignore ECONNRESET

[vstinner]

BPO	30319
Nosy	@pitrou, @vstinner, @tiran, @vadmium, @dstufft
PRs	bpo-30319: socket.close() now ignores ECONNRESET #2565 [3.6] bpo-30319: socket.close() now ignores ECONNRESET (#2565) #2566

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2017-07-05.07:36:24.789>
created_at = <Date 2017-05-09.16:00:13.568>
labels = ['3.7', 'type-bug', 'tests']
title = 'Change socket.close() to ignore ECONNRESET'
updated_at = <Date 2017-07-06.08:50:25.105>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2017-07-06.08:50:25.105>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2017-07-05.07:36:24.789>
closer = 'pitrou'
components = ['Tests']
creation = <Date 2017-05-09.16:00:13.568>
creator = 'vstinner'
dependencies = []
files = []
hgrepos = []
issue_num = 30319
keywords = ['buildbot', '3.6regression']
message_count = 28.0
messages = ['293331', '293409', '293438', '295109', '295264', '297463', '297537', '297538', '297539', '297544', '297545', '297551', '297643', '297644', '297645', '297646', '297649', '297650', '297651', '297657', '297659', '297660', '297664', '297708', '297709', '297746', '297790', '297817']
nosy_count = 6.0
nosy_names = ['janssen', 'pitrou', 'vstinner', 'christian.heimes', 'martin.panter', 'dstufft']
pr_nums = ['2565', '2566']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue30319'
versions = ['Python 3.6', 'Python 3.7']

[vstinner]

http://buildbot.python.org/all/builders/AMD64%20FreeBSD%209.x%203.6/builds/128/steps/test/logs/stdio

test_invalid_authentication (test.test_imaplib.NewIMAPSSLTests) ... SENT: b'* OK IMAP4rev1'
GOT: b'OMOH0 CAPABILITY'
SENT: b'* CAPABILITY IMAP4rev1'
SENT: b'OMOH0 OK CAPABILITY completed'
GOT: b'OMOH1 AUTHENTICATE MYAUTH'
SENT: b'+'
GOT: b'ZmFrZQ=='
SENT: b'OMOH1 NO [AUTHENTICATIONFAILED] invalid'
ERROR

======================================================================
ERROR: test_invalid_authentication (test.test_imaplib.NewIMAPSSLTests)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/usr/home/buildbot/python/3.6.koobs-freebsd9/build/Lib/test/test_imaplib.py", line 223, in _cleanup
    self.client.shutdown()
  File "/usr/home/buildbot/python/3.6.koobs-freebsd9/build/Lib/imaplib.py", line 326, in shutdown
    self.sock.close()
  File "/usr/home/buildbot/python/3.6.koobs-freebsd9/build/Lib/socket.py", line 417, in close
    self._real_close()
  File "/usr/home/buildbot/python/3.6.koobs-freebsd9/build/Lib/ssl.py", line 1052, in _real_close
    socket._real_close(self)
  File "/usr/home/buildbot/python/3.6.koobs-freebsd9/build/Lib/socket.py", line 411, in _real_close
    _ss.close(self)
ConnectionResetError: [Errno 54] Connection reset by peer

---

Ran 95 tests in 32.694s

FAILED (errors=1, skipped=2)
Warning -- threading._dangling was modified by test_imaplib
Before: <_weakrefset.WeakSet object at 0x805e2b608>
After: <_weakrefset.WeakSet object at 0x8014a25a0>
test test_imaplib failed

[vstinner]

See also bpo-30328 (test_ssl) and bpo-30315 (test_ftplib).

[vstinner]

@our SSL experts: any idea why sock.close() fails with "ConnectionResetError: [Errno 54] Connection reset by peer" in imaplib? Is that a new error? Should imaplib catch ConnectionResetError on sock.close()?

By the way, see also bpo-30329: I proposed a patch to catch the Windows socket error 10022 on shutdown() for poplib and imaplib SSL connections. No idea why this error only occurs on SSL connections.

[vadmium]

Making this an index of related reports:

bpo-30319: test_imap
bpo-30315: test_ftplib
bpo-30543: test_timeout
bpo-30328: test_ssl
bpo-27784: test_asyncore.TestAPI_UseIPv6Select.test_handle_accept, test_socketserver
bpo-30106: test_asyncore.TestAPI_UseIPv6Poll.test_handle_write

These all look like a side effect of my change to raise an error from the OS as an exception when closing a socket, bpo-26685. Only 3.6+ is affected. According to <https://www.freebsd.org/cgi/man.cgi?close%282%29\>, ECONNRESET means “The underlying object was a stream socket that was shut down by the peer before all pending data was delivered”.

It seems this is specific to Free BSD. See bug report about Posix compliance: <https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=159179\>. According to <https://forums.zeroc.com/discussion/5569/patch-to-network-cpp-for-freebsd-for-econnreset-on-close-2-problem\> this started in Free BSD 6.3 in 2006.

I suppose the options are:

1. Completely revert bpo-26685 and ignore all “socket.close” errors (my least preferred option)
2. Ignore ECONNRESET in “socket.close” (backwards compatible, could use “os.close” if you really want to check for ECONNRESET)
3. Ignore ECONNRESET in the various higher-level libraries
4. Adjust the tests to ignore the error or otherwise avoid the problem

[vstinner]

Before discussing revert, I would like experimenting fixing calls to
sock.close(). How much changes do we need to ignore exeptions on
sock.close()?

[vadmium]

I think fixing all affected calls to socket.close in the world (option 3) would be too much. I just added two new reports (bpo-30652 and bpo-30391) as dependencies. These are about testing socketserver.TCPServer. As an example, to fix the socket.close call there, I think the change would look like

class TCPServer:
    def close_request(self, request):
        try:
            request.close()
        except ConnectionError:
            # Suppress asynchronous errors such as ECONNRESET on Free BSD
            pass

Instead of that change all over the place, I am thinking option 2 would be safest. In Modules/socketmodule.c, something like

sock_close(PySocketSockObject *s)
{
    Py_BEGIN_ALLOW_THREADS
    res = SOCKETCLOSE(fd);
    Py_END_ALLOW_THREADS
    /* ECONNRESET can occur on Free BSD */
    if (res < 0 && errno != ECONNRESET) {
        return s->errorhandler();
    }
}

[pitrou]

2. Ignore ECONNRESET in “socket.close” (backwards compatible, could use “os.close” if you really want to check for ECONNRESET)

+1 from me.

[pitrou]

Please, let's not use the "dependencies" field for issues which are not actual dependencies.

[pitrou]

None of "27784,30106,30315,30328,30391,30652,30543" are dependencies of this issue.

[vstinner]

tl; dr I agree to ignore ECONNRESET-like errors on socket.shutdown() and sock.close().

I'm not sure that I understand *exactly* the problem here. In which case closing a socket fails with ECONNRESET? Can it mean that the last write() was buffered and failed?

If socket.send() is blocking, I expect that send() gets the ECONNRESET error.

If socket.send() is non-blocking, the application has to be very carefully written to handle many kinds of errors.

From a high lever point of view, I don't think that ECONNRESET is interested. I expect that a protocol at the application level doesn't rely on ECONNRESET, but a command like "QUIT". No?

For a file on disk, it's different, since write() is always buffered and close() has to flush pending write on disk.

For a network protocol, loosing the connection, loosing data, etc. is something "normal", not something execptional. That's why there are application-level commands to close cleanly a connection.

Ok, now for SSL sockets... Is it also ok to ignore ECONNRESET on sock.close() for an SSL socket?

--

ECONNRESET can occur on sock.close(), but not only: see bpo-30329, shutdown() fails can ENOTCONN on UNIX or WSAEINVAL on Windows. I modified poplib and imaplib recently to handle WSAEINVAL:

commit 83a2c28
Author: Victor Stinner <victor.stinner@gmail.com>
Date: Mon May 15 17:33:45 2017 +0200

bpo-30329: Catch Windows error 10022 on shutdown() (bpo-1538)

Catch the Windows socket WSAEINVAL error (code 10022) in imaplib and
poplib on shutdown(SHUT_RDWR): An invalid operation was attempted

This error occurs sometimes on SSL connections.