<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div><blockquote type="cite" class=""><div class="">On Jul 3, 2017, at 7:32 AM, Gelareh Taban <<a href="mailto:gtaban@us.ibm.com" class="">gtaban@us.ibm.com</a>> wrote:</div><div class=""><div class=""><p class="">> * Given that the protocol has no methods, only properties, and that `ConnectionType` is concrete, <br class="">> what behavior is `ConnectionDelegate` abstracting over? Or is it just a placeholder design? <br class="">> (If it is a placeholder, a comment to that effect might be helpful.)<br class=""><br class="">It is abstracting over different transport layers cross platform (right now, Linux, but potentially in the future, also windows etc).<br class="">I agree that the defacto is BSD sockets but this type of abstraction can allow support of other protocols such as STREAMS, etc.<br class=""><br class="">The question I guess is do we think this support is necessary?<br class=""></p></div></div></blockquote><div>We should absolutely abstract over the underlying transport—that's not even a question in my mind. My criticism is from the other direction: This design isn't really abstracting over anything, because it doesn't provide any universal operations that can be performed on any connection regardless of its underlying implementation.</div><div><div><br class=""></div><div>Think of it this way: Suppose you want to send some data through a `ConnectionDelegate`. How do you do that? There's no `send(_:)` method on `ConnectionDelegate` or anything similar; the only thing I can think of that you could do is switch on `endpoint` and call an appropriate API for each case. That's not "abstracting" over connection types in any meaningful sense.</div><div><br class=""></div><div>(Unless you're supposed to interact with the `ConnectionDelegate` through other APIs not shown here.)</div><blockquote type="cite" class=""><div class=""></div></blockquote></div><blockquote type="cite" class=""><div class=""><div class=""><p class="">> * I'm very confused by `TLSServiceDelegate` and `ConnectionDelegate` both having "delegate" in <br class="">> their names and (probably) both holding references to each other. The relationship between these two types seems very jumbled.<br class=""><br class="">TLSServiceDelegate is a delegate to the socket class.<br class="">ConnectionDelegate is misnamed and can have the delegate removed.<br class=""></p></div></div></blockquote><div>Okay, that sounds fair to me.</div><blockquote type="cite" class=""><div class=""><div class=""><p class="">> * `TLSdelegate` should be `tlsDelegate` to meet the Swift API Guidelines.<br class=""><br class=""><br class="">yes... but then the acronym wont be capitalized! :-P</p></div></div></blockquote><div>This is explicitly suggested by the API Guidelines, which show this example:</div><div><br class=""></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>var utf8Bytes: [UTF8.CodeUnit]<div class=""><br class=""></div>I don't love the way it looks, but I was there for that thread, and believe me, it could have been a lot worse. As long as we have the guidelines, we might as well follow them.</div><blockquote type="cite" class=""><div class=""><div class=""><p class="">We really need to think of a good name here :-) Any suggestions?<br class=""></p></div></div></blockquote><div>If you want to avoid the `tls` prefix, my best suggestion is to rename all of these types and members to, for instance, `SecurityServiceDelegate`, `securityDelegate`, etc. But I'd just stick with it, personally.</div><blockquote type="cite" class=""><div class=""><div class=""><p class="">> * `ConnectionType`'s `unknown` case seems rather useless. Why isn't the `endpoint` property simply Optional instead?<br class=""><br class="">I think all connections must have an endpoint so I don't think it makes sense to have it optional. You're right that the unknown case <br class="">is probbably not useful and we can probably remove it.<br class=""></p></div></div></blockquote><div>`nil` here would not mean "does not have an endpoint", but rather "does not have an endpoint representable using the `ConnectionEndpoint` type".</div><div><br class=""></div><div>On the other hand, if the idea is that the `endpoint` property is a way to directly access the underlying transport if you happen to know what it is and how to use it directly, then I think the enum is the wrong design. Every connection has *some* kind of endpoint, but we cannot enumerate all the possible endpoints ahead of time. So `ConnectionEndpoint` should instead be an empty marker protocol:</div><div><br class=""></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>public protocol Connection {</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>…</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>var endpoint: ConnectionEndpoint { get }</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>}</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>public protocol ConnectionEndpoint {}</div><div><br class=""></div><div>And we should provide a wrapper type around Int32 to mark it as a socket:</div><div><br class=""></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>public struct Socket: RawRepresentable, ConnectionEndpoint {</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>public var rawValue: Int32</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>public init(rawValue: Int32) { self.rawValue = rawValue }</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>}</div><div><br class=""></div><div>But if you're using something else, you would just mark your thing as a `ConnectionEndpoint` and return it from your `endpoint` property:</div><div><br class=""></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>extension CFSocket: ConnectionEndpoint {}</div><div><span class="Apple-tab-span" style="white-space:pre"> </span></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>class MyConnection: Connection {</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>var socket: CFSocket</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>…</div><div><span class="Apple-tab-span" style="white-space:pre"> </span></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>var endpoint: ConnectionEndpoint {</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>return socket</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>}</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>}</div><div><br class=""></div><div>If you want to use the endpoint, you `as?`-test for the types you handle, and then use the protocol methods as a fallback:</div><div><br class=""></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>func willSend(_ data: Data) throws -> Int {</div><div><span class="Apple-tab-span" style="white-space: pre;"> </span>if let socket = connection.endpoint as? Socket {</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>return try ssl_send_encrypted(socket.rawValue, data)</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>}</div><div><span class="Apple-tab-span" style="white-space:pre"> </span></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>// Fall back to the slow path</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>let ciphertext = try ssl_encrypt(data)</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>return try connection.send(ciphertext)</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>}</div><blockquote type="cite" class=""><div class=""><div class=""><p class="">> * The parameter labels in `didAccept(connection:)` and `willSend(data:)` are inappropriate; they should be omitted.<br class=""><br class="">Sorry, why are they inappropriate? I was trying to make it readable.<br class=""></p></div></div></blockquote><div>Parameter labels shouldn't duplicate information that's already present in the parameter's type. The parameter already *is* a `Connection`/`Data`; we don't need to say so again. This is all stuff from the API Guidelines: <<a href="https://swift.org/documentation/api-design-guidelines/#argument-labels" class="">https://swift.org/documentation/api-design-guidelines/#argument-labels</a>></div><blockquote type="cite" class=""><div class=""><div class=""><p class="">> * The buffer-based `willSend` and `willReceive` calls ought to use `UnsafeRawBufferPointer` and <br class="">> `UnsafeMutableRawBufferPointer` instead of passing the pointer and size as separate parameters. <br class="">> This `willSend` variant should also be unlabeled.<br class=""><br class="">Makes sense but tbh I was intending to remove those APIs and only support Data. The data type that these methods <br class="">support depend on the what the socket class will support, which as far as I understand, have not been defined yet.<br class=""><br class="">Do you think it still makes sense for us to include the Unsafe[Mutable]RawBufferPointer versions of the APIs?<br class=""></p></div></div></blockquote><div>Maybe. Since these types are low-level, they're likely to be faster than the alternatives. But manual memory management is more difficult as well, so it may not be worth it.</div><div><br class=""></div><div>Basically, I think keeping them and removing them are both plausible designs. Pick one and move on.</div><blockquote type="cite" class=""><div class=""><div class=""><p class="">> * I continue to be concerned by the `didCreateClient()` and `didCreateServer()` calls. In the example implementation, <br class="">> they simply call into methods which, to my mind, ought to be part of initialization. Large swaths of `TLSService` properties <br class="">> are mutable seemingly entirely to accommodate this half-initialized state. I think splitting the static, shareable configuration <br class="">> from the dynamic, per-connection SSL delegate (by replacing these two calls with `makeClientDelegate()` and <br class="">> `makeServerDelegate()` calls on a separate protocol) would substantially clean up this design.<br class=""><br class="">Interesting, can you give a bit more detail on this?<br class=""></p></div></div></blockquote><div>What I'm suggesting is that the protocol should be split into two:</div><div><br class=""></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>protocol TLSServiceConfiguration {</div><div><span class="Apple-tab-span" style="white-space: pre;"> </span>func makeClientDelegate() throws -> TLSServiceDelegate</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>func makeServerDelegate() throws -> TLSServiceDelegate</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>}</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>protocol TLSServiceDelegate {</div><div><span class="Apple-tab-span" style="white-space: pre;"> </span>func didConnect(to connection: Connection) throws</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>func didAccept(_ connection: Connection) throws</div><div><span class="Apple-tab-span" style="white-space: pre;"> </span></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>func willSend(_ data: Data) throws -> Int</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>func willReceive(into data: inout Data) throws -> Int</div><div><span class="Apple-tab-span" style="white-space:pre"> </span></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>func willDestroy()</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>}</div><div><br class=""></div>You would have one shared `TLSServiceConfiguration` object which you used as a factory to produce a `TLSServiceDelegate` for each connection.<br class=""><blockquote type="cite" class=""><div class=""><div class=""><p class="">I think potentially what you are suggesting might mean that the socket library needs to be split as well to client and server components. Is that correct?<br class=""></p></div></div></blockquote><div>You could do that if you wanted to, yes. Or you could combine the `makeClientDelegate()`/`didConnect(to:)` pair, and the `makeServerDelegate()`/`didAccept(_:)` pair, and then the three remaining calls would all make sense for both types:</div><div><br class=""></div><div><div><span class="Apple-tab-span" style="white-space: pre;"> </span>protocol TLSServiceConfiguration {</div><div><span class="Apple-tab-span" style="white-space: pre;"> </span>func makeClientDelegate(connectingTo connection: Connection) throws -> TLSServiceDelegate</div><div><span class="Apple-tab-span" style="white-space: pre;"> </span>func makeServerDelegate(acceptingFrom connection: Connection) throws -> TLSServiceDelegate</div><div><span class="Apple-tab-span" style="white-space: pre;"> </span>}</div><div><span class="Apple-tab-span" style="white-space: pre;"> </span>protocol TLSServiceDelegate {</div><div><span class="Apple-tab-span" style="white-space: pre;"> </span>func willSend(_ data: Data) throws -> Int</div><div><span class="Apple-tab-span" style="white-space: pre;"> </span>func willReceive(into data: inout Data) throws -> Int</div><div><span class="Apple-tab-span" style="white-space: pre;"> </span></div><div><span class="Apple-tab-span" style="white-space: pre;"> </span>func willDestroy()</div><div><span class="Apple-tab-span" style="white-space: pre;"> </span>}</div><div><br class=""></div><div>Or you could use the design I showed just above. All of those are plausible.</div></div></div><br class=""><div class="">
<span class="Apple-style-span" style="border-collapse: separate; font-variant-ligatures: normal; font-variant-east-asian: normal; font-variant-position: normal; line-height: normal; border-spacing: 0px;"><div class=""><div style="font-size: 12px; " class="">-- </div><div style="font-size: 12px; " class="">Brent Royal-Gordon</div><div style="font-size: 12px; " class="">Architechies</div></div></span>
</div>
<br class=""></body></html>