Deprecate aio::Connection

[nihohit]

aio::Connection is deprecated due to it entering erroneous state when user drops its futures before they complete (similar to OpenAI's RedisPy issue). PubSub & Monitor, which are built on top of aio::Connection, are now exposed for direct creation from a client, in order to allow us to transition their implementation to another backing connection.

[nihohit]

It should be noticed that if we don't type-erase pubsub & monitor, then replacing their internal implementation will still be semver-breaking.

[nihohit]

@jaymell I'll fix the CI errors once you give the go-ahead to the general direction of this PR, ok?

[jaymell]

I think this is a great approach and badly needed! Would love to get it out ASAP, honestly.

As for using traits, I'm not opposed to it, though also not sure it's totally necessary?

I'm also curious to see what the performance difference is of using a multiplexed connection to back this. ... If we continue to use aio::connection behind the scenes, we'll need to do our own sanity checking on the connection, possibly similar to #733.

[nihohit]

As for using traits, I'm not opposed to it, though also not sure it's totally necessary?

I don't know another way to avoid semver breaking when changing an implementation. If care about maintaining the same API, then traits aren't needed, but I don't know about a way to change types without breaking semver and without using dyn traits.

I'm also curious to see what the performance difference is of using a multiplexed connection to back this

I don't think that's possible. multiplexed connections work by pushing requests and receiving a oneshot channel carrying a Value per-request. It doesn't allow for stream responses. By the same token, it's unclear what multiplexing would mean in Pub/Sub - is it like multiple subscribers? are messages that are sent to a multiplexed connection also sent to all of its clones?

IMO aio::Connection mostly behaves like you'd expect a Pub/Sub - you have a single connection that passes messages, regardless of inputs. This is problematic if the connection is used as aio::ConnectionLike and user drops a reply future, because it causes the message queue to go out of sync, but it's a solid base for Pub/Sub.

[jaymell]

I don't think that's possible. multiplexed connections work by pushing requests and receiving a oneshot channel carrying a Value per-request. It doesn't allow for stream responses. By the same token, it's unclear what multiplexing would mean in Pub/Sub - is it like multiple subscribers? are messages that are sent to a multiplexed connection also sent to all of its clones?

Yeah point taken. I guess I'm curious if we were to bolt streaming response functionality to the multiplexed connection, could we live with one implementation of the connection. Could we potentially have an implementation for pub-sub similar to what you have here that essentially wraps the multiplexed connection but doesn't allow cloning? Just thinking out loud here.

[nihohit]

Could we potentially have an implementation for pub-sub similar to what you have here that essentially wraps the multiplexed connection but doesn't allow cloning?

Using #898 we could tweak multiplexed connection to always send on the push manager when its in PubSub mode, and use this design. The problem is that it'll make the struct behave in two completely different ways, depending on the mode of usage, which IMO means the gains from using the same struct are minor. I'm not even sure that some hypothetical future bug fix could be applied to both modes.
On the other hand, aio::Connection uses the "correct" streaming model for PubSub, and AFAIK the issues it has with stability and dropped requests aren't relevant for PubSub/Monitor modes, so IMO the minor gains of switching to hidden multiplexing are outweighed by the minor losses of not using aio::Connection, which better fits the bill.

BUT I'm saying this after very little research. This isn't a well informed, thoroughly researched take :)

[jaymell]

If you'd like to make these traits instead, I think we should do it.

[nihohit]

If you'd like to make these traits instead, I think we should do it.

@jaymell IMO we should see what's happening with #633 first, so that we don't throw a spanner in the works, there. Would you like me to open an issue on this, so that we won't release a version without addressing this?