classification
Title: asyncio: add a new Protocol.connection_failed() method
Type: Stage:
Components: asyncio Versions: Python 3.5, Python 3.4
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: gvanrossum, pitrou, python-dev, vstinner, yselivanov
Priority: normal Keywords: patch

Created on 2015-01-27 22:39 by vstinner, last changed 2015-03-18 10:49 by vstinner. This issue is now closed.

Files
File name Uploaded Description Edit
accept_error.patch vstinner, 2015-01-27 22:39 review
connection_failed.patch vstinner, 2015-01-29 00:03 review
accept_connection_failed.patch vstinner, 2015-01-29 02:09
connection_failed-2.patch vstinner, 2015-01-29 02:11 review
Messages (11)
msg234856 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-01-27 22:39
While working on the issue #23243 ("asyncio: emit ResourceWarning warnings if transports/event loops are not explicitly closed"), I saw that SelectorEventLoop._accept_connection() currently ignores errors on the creation of a transport.

When a server gets an incoming connection: it calls sock.accept(), creates a protocol, and then create the transport. It doesn't wait until the connection_made() of the protocol is called (until the transport was successfully created).

For example, for a SSL server, the client may decide to close the connection because it doesn't trust the server certificate. In this case, the SSL handshake fails at server side.

Currently, the user of the asyncio API cannot decide how to handle this failure.

I propose to call the connection_lost() method of the protocol with the exception, even if the connection_made() method of the protocol was not called (and will never be called). Attached patch implements this idea.

It's a change in the undocumented "state machine" of protocols. Before, it was not possible to switch directly to connection_lost(): there is even an assertion which ensures that it never occurs in some unit tests.

A server may log the connection failure, blacklist temporarely the client IP, etc.

Problem: Since the protocol doesn't know the transport yet, it doesn't have access to the socket, and so cannot retrieve the IP address of the client.

Maybe a new method should be added to protocols to handle this case?

How do other event loops (Twisted, eventlet, Tornado, etc.) handle failures on incoming connection?
msg234857 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2015-01-27 22:41
IMO, connection_lost() should never be called if connection_made() wasn't called. That's a breach of the API contract.

(at one point, I suggested a connection_failed() for that purpose, but it was shut down - it was in relationship to the idea of a reconnecting client, but can still be more broadly useful)
msg234860 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-01-27 22:47
FYI I opened a thread about this issue on the Tulip mailing list.

Antoine Pitrou added the comment:
> IMO, connection_lost() should never be called if connection_made() wasn't called. That's a breach of the API contract.

Yes, I agree.

> (at one point, I suggested a connection_failed() for that purpose, but it was shut down - it was in relationship to the idea of a reconnecting client, but can still be more broadly useful)

I like the "connection_failed" name. We may call
protocol.connection_failed(transport), so the protocol gets access to
the socket and so to the IP addres.
msg234864 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-01-27 23:26
Oh, accept_error.patch causes issues with the new SSL implementation. SSLProtocol.feed_data() is called before SSLProtocol.connection_made() is called. _SelectorSocketTransport constructor calls loop.add_reader() immediatly, but it only schedules a call to protocol.connection_made() with loop.call_soon().

The call to loop.add_reader() should maybe be scheduled after the call to connection_made()? To ensure that protocol methods (feed_data) are not called before connection_made() has been called.
msg234929 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-01-28 23:48
> The call to loop.add_reader() should maybe be scheduled after the call to connection_made()? To ensure that protocol methods (feed_data) are not called before connection_made() has been called.

Fixed by:
---
changeset:   94360:1b35bef31bf8
branch:      3.4
tag:         tip
user:        Victor Stinner <victor.stinner@gmail.com>
date:        Thu Jan 29 00:36:51 2015 +0100
files:       Lib/asyncio/selector_events.py Lib/test/test_asyncio/test_selector_events.py
description:
asyncio: Fix _SelectorSocketTransport constructor

Only start reading when connection_made() has been called:
protocol.data_received() must not be called before protocol.connection_made().
---

Other fix related to this issue:
---
changeset:   94358:1da90ebae442
branch:      3.4
parent:      94355:263344bb2107
user:        Victor Stinner <victor.stinner@gmail.com>
date:        Thu Jan 29 00:35:56 2015 +0100
files:       Lib/asyncio/sslproto.py Lib/test/test_asyncio/test_sslproto.py
description:
asyncio: Fix SSLProtocol.eof_received()

Wake-up the waiter if it is not done yet.
---
msg234930 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-01-29 00:03
New patch which adds a new Protocol.connection_failed() method.

The method is called when the creation of the transport failed, ie. when the connection failed, on SSL handshake failure for example.

The patch also closes the transport on connection failure (avoid a ResourceWarning with patch of the issue #23243).
msg234937 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-01-29 02:11
I splitted connection_failed.patch in two parts:

- connection_failed-2.patch: add Protocol.connection_failed() and call when the creation of a transport failed because connection_made() was called
- accept_connection_failed.patch: Fix BaseSelectorEventLoop._accept_conncetion(). On error, call connection_failed() and then close the transport.
msg234938 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-01-29 02:13
> - connection_failed-2.patch: add Protocol.connection_failed() and call when the creation of a transport failed because connection_made() was called

Oops. "... *before* connection_made() was called".
msg234970 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-01-29 13:17
New changeset c4fd6df9aea6 by Victor Stinner in branch '3.4':
asyncio: sync with Tulip
https://hg.python.org/cpython/rev/c4fd6df9aea6
msg234971 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-01-29 13:18
I commited  a simplified version of accept_connection_failed.patch, without the call to connection_failed().
msg238404 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-03-18 10:49
The consensus looks to be to reject this feature, so I close the issue. I already commits a compromise: log an error in debug mode.
History
Date User Action Args
2015-03-18 10:49:40vstinnersetstatus: open -> closed
resolution: rejected
messages: + msg238404
2015-01-29 13:18:07vstinnersetmessages: + msg234971
2015-01-29 13:17:23python-devsetnosy: + python-dev
messages: + msg234970
2015-01-29 02:13:02vstinnersetmessages: + msg234938
2015-01-29 02:11:54vstinnersetfiles: + connection_failed-2.patch

messages: + msg234937
2015-01-29 02:09:47vstinnersetfiles: + accept_connection_failed.patch
2015-01-29 00:03:43vstinnersetfiles: + connection_failed.patch

messages: + msg234930
title: asyncio: call protocol.connection_lost() when the creation of transport failed -> asyncio: add a new Protocol.connection_failed() method
2015-01-28 23:48:53vstinnersetmessages: + msg234929
2015-01-27 23:26:04vstinnersetmessages: + msg234864
2015-01-27 22:47:06vstinnersetmessages: + msg234860
2015-01-27 22:41:16pitrousetnosy: + pitrou
messages: + msg234857
2015-01-27 22:39:33vstinnercreate