[swift-server-dev] TLSService mini-release

[Brent Royal-Gordon]

> On Jul 3, 2017, at 7:32 AM, Gelareh Taban <gtaban at us.ibm.com> wrote:
> > * Given that the protocol has no methods, only properties, and that `ConnectionType` is concrete, 
> > what behavior is `ConnectionDelegate` abstracting over? Or is it just a placeholder design? 
> > (If it is a placeholder, a comment to that effect might be helpful.)
> 
> It is abstracting over different transport layers cross platform (right now, Linux, but potentially in the future, also windows etc).
> I agree that the defacto is BSD sockets but this type of abstraction can allow support of other protocols such as STREAMS, etc.
> 
> The question I guess is do we think this support is necessary?
> 
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.

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.

(Unless you're supposed to interact with the `ConnectionDelegate` through other APIs not shown here.)
> > * I'm very confused by `TLSServiceDelegate` and `ConnectionDelegate` both having "delegate" in 
> > their names and (probably) both holding references to each other. The relationship between these two types seems very jumbled.
> 
> TLSServiceDelegate is a delegate to the socket class.
> ConnectionDelegate is misnamed and can have the delegate removed.
> 
Okay, that sounds fair to me.
> > * `TLSdelegate` should be `tlsDelegate` to meet the Swift API Guidelines.
> 
> 
> yes... but then the acronym wont be capitalized! :-P
> 
This is explicitly suggested by the API Guidelines, which show this example:

	var utf8Bytes: [UTF8.CodeUnit]

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.
> We really need to think of a good name here :-) Any suggestions?
> 
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.
> > * `ConnectionType`'s `unknown` case seems rather useless. Why isn't the `endpoint` property simply Optional instead?
> 
> 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 
> is probbably not useful and we can probably remove it.
> 
`nil` here would not mean "does not have an endpoint", but rather "does not have an endpoint representable using the `ConnectionEndpoint` type".

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:

	public protocol Connection {
		…
		var endpoint: ConnectionEndpoint { get }
	}
	public protocol ConnectionEndpoint {}

And we should provide a wrapper type around Int32 to mark it as a socket:

	public struct Socket: RawRepresentable, ConnectionEndpoint {
		public var rawValue: Int32
		public init(rawValue: Int32) { self.rawValue = rawValue }
	}

But if you're using something else, you would just mark your thing as a `ConnectionEndpoint` and return it from your `endpoint` property:

	extension CFSocket: ConnectionEndpoint {}
	
	class MyConnection: Connection {
		var socket: CFSocket
		…
		
		var endpoint: ConnectionEndpoint {
			return socket
		}
	}

If you want to use the endpoint, you `as?`-test for the types you handle, and then use the protocol methods as a fallback:

	func willSend(_ data: Data) throws -> Int {
		if let socket = connection.endpoint as? Socket {
			return try ssl_send_encrypted(socket.rawValue, data)
		}
		
		// Fall back to the slow path
		let ciphertext = try ssl_encrypt(data)
		return try connection.send(ciphertext)
	}
> > * The parameter labels in `didAccept(connection:)` and `willSend(data:)` are inappropriate; they should be omitted.
> 
> Sorry, why are they inappropriate? I was trying to make it readable.
> 
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: <https://swift.org/documentation/api-design-guidelines/#argument-labels>
> > * The buffer-based `willSend` and `willReceive` calls ought to use `UnsafeRawBufferPointer` and 
> > `UnsafeMutableRawBufferPointer` instead of passing the pointer and size as separate parameters. 
> > This `willSend` variant should also be unlabeled.
> 
> Makes sense but tbh I was intending to remove those APIs and only support Data. The data type that these methods 
> support depend on the what the socket class will support, which as far as I understand, have not been defined yet.
> 
> Do you think it still makes sense for us to include the Unsafe[Mutable]RawBufferPointer versions of the APIs?
> 
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.

Basically, I think keeping them and removing them are both plausible designs. Pick one and move on.
> > * I continue to be concerned by the `didCreateClient()` and `didCreateServer()` calls. In the example implementation, 
> > they simply call into methods which, to my mind, ought to be part of initialization. Large swaths of `TLSService` properties 
> > are mutable seemingly entirely to accommodate this half-initialized state. I think splitting the static, shareable configuration 
> > from the dynamic, per-connection SSL delegate (by replacing these two calls with `makeClientDelegate()` and 
> > `makeServerDelegate()` calls on a separate protocol) would substantially clean up this design.
> 
> Interesting, can you give a bit more detail on this?
> 
What I'm suggesting is that the protocol should be split into two:

	protocol TLSServiceConfiguration {
		func makeClientDelegate() throws -> TLSServiceDelegate
		func makeServerDelegate() throws -> TLSServiceDelegate
	}
	protocol TLSServiceDelegate {
		func didConnect(to connection: Connection) throws
		func didAccept(_ connection: Connection) throws
		
		func willSend(_ data: Data) throws -> Int
		func willReceive(into data: inout Data) throws -> Int
		
		func willDestroy()
	}

You would have one shared `TLSServiceConfiguration` object which you used as a factory to produce a `TLSServiceDelegate` for each connection.
> 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?
> 
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:

	protocol TLSServiceConfiguration {
		func makeClientDelegate(connectingTo connection: Connection) throws -> TLSServiceDelegate
		func makeServerDelegate(acceptingFrom connection: Connection) throws -> TLSServiceDelegate
	}
	protocol TLSServiceDelegate {
		func willSend(_ data: Data) throws -> Int
		func willReceive(into data: inout Data) throws -> Int
		
		func willDestroy()
	}

Or you could use the design I showed just above. All of those are plausible.

-- 
Brent Royal-Gordon
Architechies

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.swift.org/pipermail/swift-server-dev/attachments/20170703/e4d8ac74/attachment.html>