On Tue, Nov 18, 2014 at 12:29 PM, Andrew Stitcher <[email protected]> wrote:
> On Tue, 2014-11-18 at 09:35 -0500, Rafael Schloming wrote: > > On Mon, Nov 17, 2014 at 4:48 PM, Andrew Stitcher <[email protected]> > > wrote: > > ... > > There are a couple things I don't like about the API after the change. > > > > 1. pn_transport_client()/pn_transport_server() don't follow our naming > > conventions, as constructors you would expect them to return distinct > > types, however they both return pn_transport_t. > > I'd prefeer to return 2 distinct types which are subtypes of a common > base. However the type system of C doesn't allow me to change the code > safely to do this and keep the same common code. > > Now that you mention it I could make typedefs: > > typedef pn_transport_client_t pn_transport_t; > typedef pn_transport_server_t pn_transport_t; > > And still be able to safely use the existing pn_transport_xxx() APIs. > IMHO this would be a *incredibly* confusing. You would either have to duplicate all the APIs that you use to actually pump bytes into/out of a transport, or you would need people to magically know that they also apply to your typedefs. > > 2. Given that we have a distinct configuration phase regardless, it is > > simply unnecessary to force people to choose client vs server up front. > We > > can simply default the mode to client and have people set it to server > > during the configuration phase if that is what they wish. > > As clients and servers are fundamentally different in usage it makes no > semantic sense to me to create a client then "turn it into" a server. > They really aren't fundamentally different in usage. Once they are configured they are just an endpoint to pump bytes into/out of. This pretty much has to be the case because that is how the socket API is structured. Having two different kinds of endpoints to pump bytes into/out of would make any I/O loop written against the transport quite unnecessarily cumbersome. > > > ... > > > As a general pattern, I prefer to stick with (2). I think it's more > > flexible/extensible and allows the addition of pretty much arbitrary > > configuration/construction parameters without ever actually breaking the > > API. As a general pattern, the alternative is IMHO really quite brittle, > > e.g. if we have another parameter we want to add in the next release we > > need to choose between a combinatorial explosion of constructors, or > > breaking the API yet again: > > > > pn_transport_websockets_server(); // confusing/bloated API > > pn_transport(SERVER, WEBSOCKETS); // breaks everytime we add a > parameter > > This is a strawman. > > I'm not suggesting that non-fundamental transport attributes like the > layers present or their configuration be added to the constructor. But I > am trying remove unneeded configuration from various IO layers if it can > be sensibly defaulted. > > And in the case of implementing something like websockets (assuming it > would be a layer) you would add it in the same way as ssl/sasl - viz > pn_websocket(pn_transport_t*); This should figure out if it as a client > or server side by getting that from the transport, possibly with a way > to override the default if (and only if) that is required. > I agree it's perhaps a poor example, but it was more to illustrate the issue with the pattern rather than the specifics. I think one of the things I am having trouble with here is figuring out what you consider to be a fundamental aspect of the transport vs what is not. This worries me because if some new fundamental aspect comes along then we're breaking the API again. > > I think if you insist upon breaking the API then option (1) is preferable > > to what you currently have, however as I said above, I don't believe > there > > is really any reason to break the API. > > I don't insist on it - I can make sure the one non-working case works: > You're still breaking it eventually even if you only deprecate stuff now. --Rafael
