Refactor interfaces and make DNS/TCP/SSL connection setup non-blocking

[vitaly-krugl]

Formalize Asynchronous Services (Event loop + comms) interface between Pika core and adapters

• pika.adapters.utils.nbio_interface.AbstractIOServices
• tests.acceptance.io_services_tests

Formalize interface between Pika core and transports

• pika.adapters.utils.nbio_interface.AbstractStreamProtocol
• pika.adapters.utils.nbio_interface. AbstractStreamTransport

Factor out communications logic into transports and make connection-establishment non-blocking (Plaintext and SSL)

• pika.adapters.utils.io_services_utils._AsyncPlaintextTransport
• pika.adapters.utils.io_services_utils._AsyncSSLTransport
• pika.adapters.base_connection.BaseConnection.connection_made, etc.

Connection-establishment using multiple connection parameters configs for fault-tolerant implementations

• pika.AMQPConnectionWorkflow
• pika.BlockingConnection.init()
• pika.adapters.base_connection.BaseConnection.create_connection()

Enable IPv6 testing on travis-ci

• .travis.yml

Summary of new features:

• Made connection brining-up (DNS, socket.connect(), SSL-handshake) non-blocking and implemented timeout support for connection bring-up.
• Legacy ConnectionParameters.socket_timeout is now used to abort socket.connect() if it doesn't complete in time.
• Added stack_timeout property and constructor arg to connection parameters (connection.Parameters/ConnectionParameters/URLParameters).
• ConnectionParameters.stack_timeout is used to time out individual TCP/[SSL]/AMQP full-stack bring-up attempts.
• BlockingConnection supports passing a sequence of connection parameters for fault-tolerant implementations.
• Asynchronous adapters support multiple connection param configs via the class method create_connection().
• ConnectionClosed is now formalized for use with the well-defined reason_code (and reason_text) which is not available in such scenarios
• ConnectionClosed subclass ConnectionClosedByClient is used when a fully-open AMQP connection is gracefully closed as the result of user's connection.close() call.
• ConnectionClosed subclass ConnectionClosedByBroker is used upon receipt of AMQP Connection.Close from the broker.

Summary of backward-incompatible improvements for v1.0.0 target release.

• Changed Connection.close() to raise pika.exceptions.ConnectionWrongStateError if connection is already closed or closing instead of simply returning.
• BlockingConnection.close() raises ConnectionWrongStateError instead of simply returning if connection is not open.
• Use pika.exceptions.ConnectionWrongStateError instead of ConnectionClosed when the connection is in wrong state for the requested operation. This makes more sense in a number of scenarios such as when connection is opening.
• Use pika.exceptions.ConnectionBlockedTimeout exception instead of ConnectionClosed with InternalCloseReasons.BLOCKED_CONNECTION_TIMEOUT.
• Get rid of pika.Connection.InternalCloseReasons.
• pika.connection.Connection now passes exception object to on_close_callback instead of reason_code/reason_text args since not all close reasons have a well-defined reason_code. This also makes it consistent with on_open_error_callback signature. Now ConnectionClosed exception is used only for cases when fully-open AMQP connection is closed by user upon receipt of AMQP Connection.CloseOK from broker and when AMQP Connection.Close is received from broker.
• When non-blocking connection bring-up is interrupted via connection.close() call (before on_open_callback is called), report ConnectionOpenAborted exception via on_open_error_callback. Previously, ConnectionClosed was reported via on_close_callback even though the connection opening never completed. I.e., if opening fails or is interrupted for any reason, it's now consistently reported via on_open_error_callback.
• Only one of on_open_error_callback/on_close_callback will be called by pika.connection.Connection, depending on whether connection reached the fully-open state before the closure. Previously, on_close_callback was called whenever on_open_error_callback was called.
• Removed pika.connection.Connection.connect() public method. Connections are not adequately tested for properly resetting state and reconnecting. Similar to socket.socket objects, just create a new instance of connection to (re)connect.
• Renamed pika.exceptions.RecursionError exception to ReentrancyError in order to avoid naming conflict with the built-in RecursionError exception.

Other implementation changes:

• Make error info in on_open_error_callback orthogonal: now always pass exception object (versus formerly exception or string).
• Lots of new tests and hardening of internal test frameworks.

Also, as consequence of refactoring:
Fixes: #528
Fixes: #647
Fixes: #889
Fixes: #960
Fixes: #970
Fixes: #985

[vitaly-krugl]

STATUS UPDATE:

1. Passes almost all existing tests. Waiting for resolution of PR Fixed leaking of ChannelClosed exception back through the callback from SelectConnection that notified BlockingChannel about broker-initiated Channel.Close #996 which should facilitate the passing of the 5 remaining blocking connection acceptance tests that are presently failing.
2. Three unit tests have been skipped, need to be fixed:
   • tests/unit/blocking_connection_tests.py: @unittest.skip('TODO FIX ME BEFORE MERGING IN MASTER (HANGS)')
   • tests/unit/blocking_connection_tests.py: @unittest.skip('TODO FIX ME BEFORE MERGING IN MASTER')
   • tests/unit/connection_tests.py: @unittest.skip('TODO FIX ME BEFORE MERGING IN MASTER')
3. Need to complete acceptance test for async services adapters.
4. Need to implement specific tests for
   • _AsyncPlaintextTransport
   • _AsyncSSLTransport
   • _AsyncSocketConnector
   • _AsyncStreamConnector
   • base_connection._TransportManager
5. Not sure what to do with TwistedProtocolConnection, since one of its main reasons for existence is to provide non-blocking connection establishment in Twisted, which this pull request accomplishes for all adapters universally. That and the fact that it changes the connection and channel interfaces (returns deferreds) without providing anything close to sufficient test coverage makes me want to remove it from pika for v1.0.0, leaving TwistedConnection as the only twisted connection adapter, which also sports an overridden API that makes use of deferreds.
6. Need to add a "channel ready" callback to Connection.channel().

cc @lukebakken

[vitaly-krugl]

These changes are temporary, will be reverted. They help me get a better picture of what is failing.

[vitaly-krugl]

FOOD FOR THOUGHT:

1. Both twisted and asyncio (which borrows from twisted) frameworks have the concept of Protocol and Transport which I like. This PR makes this split internally, but I am beginning to think that it might be good to formalizing as part of the API such that any pika asynchronous connection is a Protocol that can be mated with an externally-created Transport.
   • This facilitates less error-prone AMQP connection setup - presently, we reuse the same Connection instance when socket or SSL connection attempts fail. This approach relies on resetting the connection's properties to the pristine state, which is error-prone and and doesn't have test coverage. Ideally, a user requests a Connection to be created and gets a Future (or another native construct) which notifies the user when the Connection has been set up and provides it to the user.
   • Makes it more natural for users to implement their own connection workflows, if desired.
   • Makes it cleaner to integrate with the native Transport/Protocol relationship of the given asynchronous framework.
2. The current callback-based ConnectionAPI is clumsy when it comes to integrating with Future-based frameworks (e.g., asyncio, twisted). The user can certainly wrap the callbacks in native Futures and the like, but it's not clean. Wouldn't it be nifty if pika core integrated naturally into the native I/O framework (returning Futures and the like) without having to create shims that override the many of the Connection and Channel methods? And if we could test them generically. E.g., presently, TwistedProtocolConnection attempts to alter the Connection/Channel interface to integrate natively with twisted using Defferreds, but there is no test coverage for it whatsoever, while the rest of the callback-based adapters are getting tested generically via a single shared async test suite.

[vitaly-krugl]

UPDATE:

1. All existing unit/acceptance tests are now passing once again.
2. The three skipped tests cited in the prior update have been addressed
3. PR Fixed leaking of ChannelClosed exception back through the callback from SelectConnection that notified BlockingChannel about broker-initiated Channel.Close #996 has been accepted in master and merged into this PR
4. Need to complete acceptance test for async services adapters.
5. Need to implement specific tests for
   • _AsyncPlaintextTransport
   • _AsyncSSLTransport
   • _AsyncSocketConnector
   • _AsyncStreamConnector
   • base_connection._TransportManager
6. Not sure what to do with TwistedProtocolConnection, since one of its main reasons for existence is to provide non-blocking connection establishment in Twisted, which this pull request accomplishes for all adapters universally. That and the fact that it changes the connection and channel interfaces (returns deferreds) without providing anything close to sufficient test coverage makes me want to remove it from pika for v1.0.0, leaving TwistedConnection as the only twisted connection adapter, which also sports an overridden API that makes use of deferreds.
7. Need to add a "channel ready" callback to Connection.channel().

[vitaly-krugl]

@lukebakken, in examining the build/test output log, I noticed the following message about erlang crash at the very end of the log. See in https://travis-ci.org/pika/pika/jobs/356647867, for example:

Done. Your build exited with 0.
erl_child_setup closed
Crash dump is being written to: /home/travis/build/pika/pika/rabbitmq_server-3.7.4/var/log/rabbitmq/erl_crash.dump...

[vitaly-krugl]

Users have been asking for this for a while. This PR makes it easy to provide such functionality.

[lukebakken]

@vitaly-krugl I'll try to find a nice chunk of time to look at this tomorrow. Thanks!

[vitaly-krugl]

I know what you mean :)

…

On Thu, Apr 19, 2018, 9:40 PM Luke Bakken ***@***.***> wrote: @vitaly-krugl <https://github.com/vitaly-krugl> I'll try to find a nice chunk of time to look at this tomorrow. Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1002 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABX9Kqti3ouO3yIx6w3sxwCL5_Nv9GJeks5tqWa6gaJpZM4Sy9uO> .

[vitaly-krugl]

@lukebakken, I just made a couple of API cleanup changes and rebased TwistedProtocolConnection on pika.connection.Connection instead of BaseConnection, since that makes a lot of sense.

Of course, since TwistedProtocolConnection's authors included no tests, we have no idea whether it ever worked and whether it still works.

Review away... :)

The changes are in separate commits in case you already started with the review.

[lukebakken]

I was surprised that a method called create_streaming_connection is described with "Perform SSL session establishment"

[vitaly-krugl]

@lukebakken, as mentioned at the top of the script in its docstring, I modeled the interface on asyncio. create_streaming_connection() is a subset of the behavior of asyncio's AbstractEventLoop.create_connection(). The rationale is that a protocol (AMQP connection in our case) needs a stream. The stream might be TCP or SSL/TLS on top of TCP. The protocol doesn't care which one. It's up to the transport to abstract away the differences. Pika's new AbstractIOServices.create_streaming_connection() takes care of the details, just as the comparable method in asyncio.

[lukebakken]

Thanks for clarifying.

[lukebakken]

Just a couple comments, I'm still reviewing. Tests pass on my local machine without any orphaned fd messages 😄

[vitaly-krugl]

@lukebakken, I fixed resource leaks in pika tests. Also, updated README.rst.

[hairyhum]

@vitaly-krugl is there any usage examples in examples directory and docs in docs directory which should be updated after this change?

[vitaly-krugl]

@hairyhum

is there any usage examples in examples directory and docs in docs directory which should be updated after this change?

Yes, good point. I will look through the examples and update the ones that are affected.

[lukebakken]

Should the TODO items be taken care of? Other than that, 👍 on this pull request.

[vitaly-krugl]

@lukebakken, I will follow up on TODOs and @hairyhum's feedback from home.

[vitaly-krugl]

@lukebakken @hairyhum: I updated the usage examples via commit 8f90e34.

[vitaly-krugl]

@lukebakken, regarding

Should the TODO items be taken care of?

There is nothing pressing about the new TODOs. Just food for thought at this time. If you're happy with the state of things, please go ahead and approve this pull request and we'll get it merged. Thanks!

[lukebakken]

Nice work @vitaly-krugl

[vitaly-krugl]

@lukebakken, thanks for reviewing - I know it was a job and one half :)