Skip to content
This repository has been archived by the owner on Nov 23, 2021. It is now read-only.

Add HTTPServer so we don't vend BlueSocketHTTP #26

Merged
merged 1 commit into from Aug 23, 2017

Conversation

ianpartridge
Copy link
Contributor

This PR adds a simple HTTPServer protocol, and an implementation of it called SimpleHTTPServer. The purpose of this is so that people can use this package to run an HTTP server without having to refer to BlueSocket related types. In the future, we want to remove all of those from the package and we can't do that if we've vended them as public API.

It also renames the WebApp and WebAppContaining types (which didn't have much support) to HTTPRequestHandler and HTTPRequestHandling.

@seabaylea
Copy link
Member

@swift-server/stakeholders any opinions of this as a very early implementation of an API to create/start the HTTP Server itself?

Copy link
Member

@tanner0101 tanner0101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

public protocol HTTPServer : class {

/// Start the HTTP server on the given `port`, using `handler` to process incoming requests
func start(port: Int, handler: @escaping HTTPRequestHandler) throws
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bind also accepts an address name. should we support that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Golang's net/http/Server has an Addr string field which seems to encapsulate both the address name and port. I'm not sure whether there's a standard format for this that we could/should use?

Copy link
Member

@adellibovi adellibovi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👏 !

/// A basic HTTP server. Currently this is implemented using the BlueSocket
/// abstraction, but the intention is to remove this dependency and reimplement
/// the class using transport APIs provided by the Server APIs working group.
public class SimpleHTTPServer {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure to understand the purpose of this public class. If it is something that devs are supposed to use, wouldn't it be better to call the protocol HTTPServerType and this class just HTTPServer?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to have a public constructor. And agreed - I'd prefer the concrete server class to be just HTTPServer

@ianpartridge
Copy link
Contributor Author

I've updated the PR to rename SimpleHTTPServer to HTTPServer, and I've added a default initializer to HTTPServer. More feedback very welcome.

@ianpartridge
Copy link
Contributor Author

Question for people: do we want to mark HTTPServer as final?

@aleksey-mashanov
Copy link
Contributor

Personally, I not really sure what universal API for initializing http servers should exist. Of course it can cover simple handmade http servers (maybe even both sync and async), but advanced configuration will be problematic and it can't cover http servers what already exist (think about uwsgi, nginx, httpd, ...) - they usually start using configuration file and then pass some control to application.

May be Tests directory is a better place for this HTTPServer?

@seabaylea
Copy link
Member

The aim of the HTTPServer API is to provide a simple out of the box HTTP server, similar to what is provided as part of the core APIs in other languages. That doesn't preclude you putting a HTTP/reverse proxy server infront.

@aleksey-mashanov
Copy link
Contributor

Out of the box http server is good feature but it should be placed in a separate module to not inject additional dependencies into API's one.

Some more questions:

  • Do we really need HTTPServing protocol in this case?
  • Which one sync or async (both?) http server should come out of the box?

@seabaylea
Copy link
Member

Yes, the HTTPServing protocol is somewhat superfluous given that its really a concrete implementation.

The intent is to provide an async server out of the box. Its hard to argue that the current and future concurrency model for Swift is not async - Dispatch is provided as part of the existing Swift toolchains, and large swathes of the available APIs are async based. Additionally both the Swift 5 manifesto and the recent async/await proposal points to that as the ongoing direction.

@helje5
Copy link
Contributor

helje5 commented Aug 22, 2017

Its hard to argue that the current and future concurrency model for Swift is not async

I agree with that, if you are willing to add 'default' ;-) What is wrong with using Swift to write Apache modules, Android stuff, Outlook plugins, or whatever. Nothing. There is little use in tying Swift to a specific setup.

Dispatch is provided as part of the existing Swift toolchains

And those are super limited. Even the Linux implementation is, well, you probably know the issues much better than I do ;-)

Dispatch is in fact a major issue with secondary toolchains (BSD? Raspi? Other IoT etc).

Also specifically for Dispatch, that Tesla^d^d^d^d Google guy on Concurrency in Swift:

The one problem I anticipate with GCD is that it doesn't scale well enough: server developers in particular will want to instantiate hundreds of thousands of actors in their application

That is something which is bothering me as well (but may not affect the core choice for async, i.e. one could use libuv too).

In general my opinion is that having a basic http server lib is nice in any case (I really don't expect that serious server frameworks are going to use this for plenty of reasons). So maybe lets just get this done, but lets try to make it so, that other implementations can still use the same or similar API.

@seabaylea
Copy link
Member

Thanks Helge. I'm going to merge this as-is: the remaining questions are around the need for the HTTPServing protocol and whether or not to mark as final, which I think we can deal with separately.

@seabaylea seabaylea merged commit 92f3ee4 into swift-server:develop Aug 23, 2017
@ianpartridge ianpartridge deleted the httpserver branch August 23, 2017 09:48
@weissi
Copy link

weissi commented Aug 23, 2017

@seabaylea

Its hard to argue that the current and future concurrency model for Swift is not async

but from what I can tell the current implementation is synchronous and blocking. Will this change soon? I think we should definitely do that before pitching to swift-evo.

@ianpartridge ianpartridge mentioned this pull request Aug 23, 2017
@seabaylea
Copy link
Member

@weissi that's the intent, yes - any help you can provide would be greatly appreciated!

seabaylea pushed a commit that referenced this pull request Aug 29, 2017
This enables the tests to run in Linux again
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants