[swift-server-dev] HTTP API Sketch v2

[Johannes Weiß]

Hi,

> On 7 Apr 2017, at 12:18, Helge Heß via swift-server-dev <swift-server-dev at swift.org> wrote:
> 
> On 07 Apr 2017, at 12:24, Johannes Weiß <johannesweiss at apple.com> wrote:
>>> - concrete types instead of protocols for HTTPRequest
>>> - bad for layering this on top of alternative
>>>  implementations
>>> - it can still come with a default imp
>> 
>> ok, maybe we should offer both types of API?
> 
> That is my thinking, yes. A protocol and then a concrete default implementation to get people going.
> 
>>> - also: you could make NSURLRequest/Response support it
>> I don't think they're very useful in their current state. For example:
>> 
>> - headers is a NSDictionary<NSString *,NSString *> which isn't expressive enough
>> - many properties that don't universally make sense like
> ...
>> - also NSURLRequest is also a concrete type so your points from above also apply there, right?
> 
> You misunderstood me. I’m saying that `HTTPRequest` should be a protocol and that Foundation.URLRequest could be one implementation conforming to it. And subsequently work with all APIs using the new S3 `HTTPRequest`.

I see, that makes sense. I think we should discuss the value type vs. protocol types separately though.


>>> - you should probably not add ‘convenience’ to this, that
>>> will be done by frameworks using it in whatever they
>>> consider ‘convenient’
>> that was more for the benefit of the readers of this mailing list. But yes, in the actual API, I'd leave that comment out.
> 
> If it is a protocol I’d leave the whole method out! :-) More like:
> 
>  protocol Writer {
>    func writeBody(data: DispatchData)
>  }
>  extension Writer { // convenience
>    func writeBody(data: Data)
> 
>  }

makes sense.


>>> - or it other words: does providing a `write` w/o a
>>>  completion handler give performance benefits and
>>>  is part of the API because of this, or is it just
>>>  `write(..) { _ in }`. I could see that it can be a
>>>  perf advantage (no setup of an escaping closure
>>>  necessary), but then it should not be marked as
>>>  ‘convenience’
>>> - same for Data vs DispatchData. Decide on just one?
>> 
>> hmm, if you happen to have a DispatchData it does give you performance benefits to hand that down. So in our implementation I started off with only DispatchData but it then turned out that it's convenient to also being able to write data directly. Especially before we had our higher level web frameworks on top of that. But maybe that's not needed.
> 
> If Data doesn’t replace DispatchData eventually, I think the Foundation team should provide cheap ways to convert between the two. It is a little weird that there are two of those …
> 
> 
>>> - maybe such:
>>> 
>>>   func writeTrailer(key: String, value: String)
>>> 
>>> should be
>>> 
>>>   func writeTrailer(key: UnsafePointer<CChar>,
>>>                     value: UnsafePointer<CChar>)
>>> 
>>> this also works with strings out of the box while
>>> allowing applications not using Strings for protocol
>>> data ;->
>> 
>> will work with string _literals_ not Strings, maybe we should have both?
> 
> It works with any String:
> 
>    func printIt(_ key: UnsafePointer<CChar>) {
>      let s = String(cString: key)
>      print("key is \(s)”)
>    }
>    var abc = “hello”
>    abc += “ world”
>    printIt(abc)

you're right, my bad! However that's not cheap because it will need to copy the String. A C string is \0 terminated contiguous memory and a Swift string might not be \0 terminated in memory so it'll need to make a copy which is not ideal.


>>> - I guess all ‘write’ functions need a completion
>>> callback? (including writeTrailer, writeResponse)
>> 
>> in my experience having that on the done() handler is enough. The amount of data written in trailers/response (headers)/... is small enough that you can do it regardless.
> 
> Maybe. Not sure, depends a little on the async framework in use. A header can certainly be big enough to overflow the socket buffer and block.
> Nginx uses 40 bytes per connection? I’m sure they wouldn’t want to be forced to buffer a 700 byte header block :-)
> 
> Not sure, I guess you are right.
> 
> 
>>> - I’m still a little worried about using `enum`s for
>>> an evolving protocol. That will look nice now but
>>> will fall apart in the future as we can’t extend
>>> them w/o breaking apps
>> 
>> which of the enums will be necessary to evolve?
> 
> All of them. E.g. PATCH is pretty new, or MKCOLLECTION. If a new method or HTTP status becomes ubiquitous, it will quickly get weird, all code will look like:
> 
>  switch method {
>    case .get: …
>    case .put: ...
>    case .delete: …
>    case custom(“BATCH”): // wtf! so ugly
>  }
> 
> Makes your pretty Swift code really ugly :-) Int’s plus constants are better for stuff that can change.

we're talking about quite a low level HTTP library here. Not many people would actually use it directly I suppose. URL target matching is also really ugly. You'd certainly want to support http://example.com//foo//bar.html so the target would be "//foo//bar.html" so you can't use string comparison.

So I feel it's good enough for a low-level framework.



>> Both have a `custom` case now for everything else. And most use-cases would use higher-level frameworks anyway.
> 
> Well, ideally a framework could reuse common stuff like the HTTPMethod type. It looks a little stupid to wrap such :-)

hmm, I'd wrap it to not export low-level implementation details.


>>> - Talking enums. You use them for status and method,
>>> why not for header keys? I see _big_ performance
>>> advantages in that.
>> that's surprising, what exactly is giving you the big advantages and how much is big? Is it the storage or what exactly?
> 
> - storage (essentially a byte vs the full String thing)
> 
> - storage reuse (are you going to reconstruct the String
>  every time you parse a header? or have a thread safe
>  intern map?) No issue with a byte.
> 
> - ARC (potentially, much more likely than for Ints ;-)
> - comparison speed - compare two bytes (a single
>  assembly instruction) vs comparing two Strings,
>  potentially unique ones with all the checks involved.
>  Order of magnitude more assembly instructions (I guess ;-)

that can all be implemented in HTTPHeaders as an implementation detail, right? When constructing it'd see that it's for example the 'Host' header and map it to some internal constant.


> That is also a reason why I’m in favour of supporting value
> types instead of just string. Converting your content-length
> into a String is a little stupid. That can be done at the
> C level below much faster.
> 
> As I said, I suppose people that need performance may be doing their own thing anyways. But it would be cool if it would be great out of the box :-)
> BTW: I’m not necessarily thinking high-scale cloud servers, I’m thinking more about stuff running on Rasπ’s.
> 
> 
>>> - Having the stream support ‘HTTPResponse’ looks like
>>> too high level for your specific design. You can keep
>>> that object and add a convenience method to put such
>>> on the stream.
>> 
>> as all the others above, I think very good discussion points. Anyone else having opinions?
>> 
>> 
>>> - also: HTTPResponse vs HTTPResponseHead[er?]
>> 
>> sure, why not? ;)
> 
> 
> I’d like to mention again that in HTTP/2 methods are just headers. Maybe the stuff should be modelled after that?
> 
>  request[.method] == .put

I do believe that people expect a request method field for HTTP/1.

-- Johannes