Redesign of the S2Connection class

[jorritn]

The current implementation of S2Connection was created when we didn't have a clear idea about the RM and CEM running as client or server. Now we specified that, it's time to revise the S2Connection class because it is designed to run the RM as a client. Preferably, we should create a communication channel agnostic S2Connection by separating S2Connection logic (like waiting for a reception status message) from the websocket stuff (see #110).

[lfse-slafleur]

@jorritn
I think it makes a lot of sense to combine this with providing an async interface next to the current sync interface for S2Connection: #139

Are there any other communication layers being designed or prepared next to websockets?

[jorritn]

Yes, I agree it would be a good idea to combine this with providing an async interface as well.

On the topic of another communication layer, there is currently nothing specific we're working on. However, although the WebSockets meet most of our requirements really well, it's not a very good fit when there are many connections between cloud instances of the same providers (one being the CEM and the other the RM). That's why we foresee to specify a new communication layer rather shortly. Also beware that we have designed the pairing specification such that multiple communication protocols are supported.

[lfse-slafleur]

Yes, I agree it would be a good idea to combine this with providing an async interface as well.

On the topic of another communication layer, there is currently nothing specific we're working on. However, although the WebSockets meet most of our requirements really well, it's not a very good fit when there are many connections between cloud instances of the same providers (one being the CEM and the other the RM). That's why we foresee to specify a new communication layer rather shortly. Also beware that we have designed the pairing specification such that multiple communication protocols are supported.

@jorritn Nice, thanks! That provides some good context. Also a clear headsup to keep the pairing completely from the other protocol implements.

[lfse-slafleur]

@jorritn @wcoenraads I am going to work on this shortly. Hit me up if you guys have any preferences/ideas you want incorporated

[lfse-slafleur]

Duplicate of #110

[lfse-slafleur]

A pretty significant change may be incoming in the interface of S2Connection in s2-python by splitting it into an async and sync version and also factoring out the websocket medium into a customizable medium. Do you feel like we should keep the old S2Connection interface for legacy purposes or do you think a break would be okay?

See also:
#139
#110
#135

[philipptrenz]

Now we specified that, it's time to revise the S2Connection class because it is designed to run the RM as a client.

I would like to point out that we have chosen the opposite approach: For us, the RM is the server. I would therefore ask that the revised interface continue to allow this.

Details on how we use that for our S2 over D-Bus communication are available here: #110 (comment)

[jorritn]

A pretty significant change may be incoming in the interface of S2Connection in s2-python by splitting it into an async and sync version and also factoring out the websocket medium into a customizable medium. Do you feel like we should keep the old S2Connection interface for legacy purposes or do you think a break would be okay?

See also: #139 #110 #135

IMHO the current S2Connection is currently very limiting in use so I think it makes sense to rethink how S2 connections are created taking into account that it can either be a CEM or RM and abstracting away the communication protocol. Just as is proposed in issues you're referring to. That means that in my opinion we should bring the new S2 Connection out under a new 'major' version of the library that indeed breaks the API. Btw, according to semver we're still having a beta version so we'll have to see how much ready we think the library is when deciding if that's good moment to release v1.0.0 or that it will be a 0.x.0 release. Anyways, I am fine with breaking changes.

[lfse-slafleur]

@jorritn @wcoenraads I am thinking to change the design slightly but importantly. Instead of an S2Connection --has a--> S2Medium to S2Medium --leads to one or more--> S2Connections (sync or async). This allows a (websocket/mqtt/d-bus/whatever) server to listen for new connections and spin off an S2Connection. Could you confirm if you like this change?

[wcoenraads]

Does this mean that an S2Connection is always created from an S2Medium? So instead of creating a connection directly, I would have a WebSocket S2Medium (which can be both a WS client and server), and if I want to connect or receive a connection I call a method on that medium to do so.

I think I like that change, it makes sense to me. It might not be entirely intuitive to users that creating and keeping around an S2Medium means you're running e.g. a WS server, but that can be clarified with documentation and examples. One potentially tricky aspect is that the medium can be (is?) both a client and server, and while that difference doesn't matter for S2 it does mean something for your flow control (actively creating a connection vs responding to an incoming connection).