Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discussions: http server responding way #211

Closed
keroxp opened this Issue Feb 21, 2019 · 14 comments

Comments

5 participants
@keroxp
Copy link
Contributor

keroxp commented Feb 21, 2019

on #188, I changed responding way for serve(). This changes were incompatible with previous way. So these changes temporally reverted until v0.3.0 is released. Give us your (developer) opinions about the ideal responding way of http server.

Current

export class ServerRequest {
  url: string;
  method: string;
  proto: string;
  headers: Headers;
  conn: Conn;
  r: BufReader;
  w: BufWriter;
 *bodyStream()
  body(): Promise<Uint8Array> 
  respond(r: Response): Promise<void> 
}
export async function* serve(addr: string): AsyncIterableIterator<ServerRequest>
for await (const req of serve(":80")) {
  req.respond(...)
}

Next

export type ServerRequest = {
  /** request path with queries. always begin with / */
  url: string;
  /** HTTP method */
  method: string;
  /** requested protocol. like HTTP/1.1 */
  proto: string;
  /** HTTP Headers */
  headers: Headers;
  /** matched result for path pattern  */
  match: RegExpMatchArray;
  /** body stream. body with "transfer-encoding: chunked" will automatically be combined into original data */
  body: Reader;
};

/** basic responder for http response */
export interface ServerResponder {
  respond(response: ServerResponse): Promise<void>;

  respondJson(obj: any, headers?: Headers): Promise<void>;

  respondText(text: string, headers?: Headers): Promise<void>;

  readonly isResponded: boolean;
}
export async function* serve(addr: string, cancel: Promise<any>): AsyncIterableIterator<{req: ServerRequest, res: ServerResponse}>

export type HttpHandler = (req: ServerRequest, res: ServerResponder) => unknown;

export interface HttpServer {
  handle(pattern: string | RegExp, handler: HttpHandler);

  listen(addr: string, cancel?: Deferred): Promise<void>;
}
for await (const {req, res} of serve(":80") {
  res.respond(...)
}

I did these changes for some reasons but mostly for new HttpServer. It might not be suitable for the lowest api.

@ry

This comment has been minimized.

Copy link
Contributor

ry commented Feb 21, 2019

At the lowest-level we ideally operate with only Uint8Array. I think it's nice if the lowest-level HTTP server maintains this - so - for example respondJson seems inappropriate for //http/server.ts in my mind and rather belongs to a framework.

What if we created something like //http/friendly_server.ts - which has a bit more usability than the raw server.ts ?

@ry

This comment has been minimized.

Copy link
Contributor

ry commented Feb 21, 2019

Here's how I view the //http/server.ts module

Maybe we can add this to the top of the file to clarify its purpose

in-scope:

  • accepting connections
  • parsing http/1.1 and http/2
  • keep alive
  • streaming uploads and downloads
  • setting some headers - e.g. "Connection: keep-alive"
  • trailing headers (eventually)
  • transfer encoding chunked
  • giving users some way to handle upgrade

out-of-scope:

  • setting the "Server:" header
  • URL routing
  • parsing/serializing http message bodies like JSON and utf8 strings
  • web sockets

What do you think? Are there others we can add?

@kitsonk

This comment has been minimized.

Copy link
Contributor

kitsonk commented Feb 21, 2019

I feel I generally aligned with Ry's in and out of scope.

For in-scope, "setting some headers" would need a bit more definition for me. I think we would be explicit. Setting things like content-length can be problematic and address higher order concerns, like if you are responding with an empty body and no content length, is that because you expect to stream the body in chunks or is it because you forgot to set the header?

In the request, match is clearly a higher order framework concern and I would consider it out of scope. In oak, I converted path-to-regex to TypeScript/Deno and would be totally comfortable that being added to std as it is like media types stuff, generally useful. It would be better to add that and not explicitly utilise that in http/server.

I am comfortable with the .body interface once I got my head around it, but it still presumes some things. I think there should be a way to access the body at a more lower level.

If we have a Reader for consuming the body, why don't we have a Writer interface for the response?

@bartlomieju

This comment has been minimized.

Copy link
Contributor

bartlomieju commented Feb 22, 2019

For in-scope, "setting some headers" would need a bit more definition for me. I think we would be explicit. Setting things like content-length can be problematic and address higher order concerns, like if you are responding with an empty body and no content length, is that because you expect to stream the body in chunks or is it because you forgot to set the header?

@kitsonk @ry I agree with the scope 100%, but IMHO we should handle situation when pass neither body nor content-length header is provided. It should "just work" for simple usages and provide enough config to be able to stream body (which I believe we already do), close the connection and handle more complicated situations.

I'm a fan of current respond API, it's simple enough, but Response interface can be extended to host needs described above.

@akircher

This comment has been minimized.

Copy link

akircher commented Feb 22, 2019

Thanks for asking! As the http server will likely be the first experience most developers have with Deno, I do think having the API really well thought out is a great idea.

My opinions follow:
I like req of serve() better than {req,res} of serve because I think res.respond() is redundant and the ability to write {headers, body, ...req} of serve() would just be awesome.

I think I saw @bartlomieju mention this idea. But I think that return Promise<Stream|Object> would be better and more modern than a respond() method. I don't want frameworks to resort to middleware monkey patching a respond method, when a promise chain could be very graceful. Although I am unsure how the HTTP2 Server Push would work, maybe req.push()

To the extent that Deno follows browser compatibility, I would love for Deno to make the interface similar (in terms of handling headers, etc) to the fetch api.
I like @kitsonk's idea that under the hood everything should be based on Reader and Writer APIs.

@keroxp

This comment has been minimized.

Copy link
Contributor Author

keroxp commented Feb 23, 2019

Thank you for opinions.

in #188, I added three different improvements. Changes within serve() are related to others. Apart from serve() and ServerRequest, would you approve new features below?

1. Making serve() cancellable with deferred

/**
 * iterate new http request asynchronously
 * @param addr listening address. like 127.0.0.1:80
 * @param cancel deferred object for cancellation of serving
 * */
export async function* serve(
  addr: string,
  cancel: Deferred = defer()
)

This aims to make it possible to cancel accept await from outside. It is particularly useful at testing. Now we can run e2e http test without sub processes.

2. Making middle level http server interface

As @kitsonk said, we clearly shouldn't have high level module for creating http server. But I think some middle level server module is needed apart from serve(). serve() and related functions (writeResponse(), readRequest()) should be the lowest http server interface in std. However, It is important that there is router-like server module without external dependencies (except std) like go's ServerMux.

We need:

  • simple routing map with handlers
  • easy way for responding json, text, file, html
export interface HttpServer {
  handle(pattern: string | RegExp, handler: HttpHandler);
  listen(addr: string, cancel?: Deferred): Promise<void>;
}

I'm aligned with @ry about server.ts scope. friendly_server.ts is what "2" aims to.

as @akircher said, making server-side compatible with fetch is quite interesting. I have implemented deno-fetch and deno-streams, deno version of WHATWG fetch and streams standard.

However by doing that, I've realized that WHATWG standard and deno eventually can't be compatible if deno keep having incompatible api with browsers. Especially, file system and stream api are very incompatible with deno's philosophy.ReadableStream and Reader are designed by totally different context. If we decide to take higher compatibility with browsers about http, we'll inevitably abandon an advantage of Reader/Writer. I don't like WHATWG streams standard as it is very complicated to use and not typescript friendly. Additionally, even now, there are no browsers that is completely compatible with the standard. But even so, http server compatible with browsers is interesting idea IMO.

@kitsonk

This comment has been minimized.

Copy link
Contributor

kitsonk commented Feb 23, 2019

I will respond more later, as I am mobile, while we have Defer, I don't think it is really clean way. I would like to see a PromiseCancellable based on https://github.com/tc39/proposal-cancellation. While it is stage 1, it should be polyfillable and the sponsors are quite core to TC39 so I suspect we will see it eventually.

@akircher

This comment has been minimized.

Copy link

akircher commented Feb 23, 2019

@keroxp if we do a friendly_server.ts can we make it the same API as server.ts? I mean rather than going with your interface HttpSever you keep it as req of httpServer(...). I like the idea of consistent apis between the two even though yours would have more bells & whistles.

I also agree with @kitsonk on using a upcoming standard for cancellable

@kitsonk

This comment has been minimized.

Copy link
Contributor

kitsonk commented Feb 24, 2019

As far as part 2, we rejected oak (#144) because it was a middleware and routing framework and in the end agreed that it shouldn't be part of std. I don't see what has changed.

@keroxp

This comment has been minimized.

Copy link
Contributor Author

keroxp commented Feb 24, 2019

@akircher If api is identical, serve() is enough. I doubt idea itself to use AsyncIterableIterator as request routine for general purpose server. We should pack all props and method into req object.
It may be desirable for the lowest level api but not for middle level. And routing logic can be mapped.

@kitsonk I know promise cancellable was withdrawn by some reason. I don't understand how to use cancellable promise as a canceller for serve(). break is only way to stop async iteration within for await loop. Do you mean to use cancellable promise instead of deferred promise? I prefer deferred rather than cancellable promise as it is just promise. It is very simple. An advantage of pure promise is that user can put even non-deferred promise to serve().

As I described, what we need is minimal, middle level (near to low, far from high). Your PR had higher level features than what I propose. I purely want to have ServerMux like router in std that is thin wrapper of serve. As you can see in #188 , It is composed of just 40 lines.

Why I disagreed your PR is that was much rich framework and it must be not desirable for framework growth. And as there are other http server frameworks that are growing, it is not desirable to treat one of those as a standard. I didn't disagreed with some routing logic in std.

@kitsonk

This comment has been minimized.

Copy link
Contributor

kitsonk commented Feb 24, 2019

If not cancellable promises, we should just have a .cancel() then. Exposing a deferred is a really strange API.

I think routing of any sort is a framework. If we are not including frameworks in std we need to stop there. Having an API, like path-to-regex as part of std which someone could easily create a routing solution on top of serve() is as far as we should go. Having the content negotiation APIs that are in oak in std might also make sense, but as soon as you merge those APIs into something else, it becomes a framework. The lines of code is immaterial. Providing examples of how the APIs could be stitched together is different than actually doing so.

@keroxp

This comment has been minimized.

Copy link
Contributor Author

keroxp commented Feb 25, 2019

@kitsonk

routing of any sort is a framework.

We may be not able to share point of view about a framework. I think having minimal features or much is a watershed. I believe new friendly_server.ts is minimal enough. The lines of code is important.

@ry How do you think? It seems that nothing is better than something for them. No landing point is seen. It may be waste to continue discussion even if there is someone who want to have these features.

@keroxp

This comment has been minimized.

Copy link
Contributor Author

keroxp commented Mar 10, 2019

Ok, I understand that there are no better way from current implementation about http server. I'm sorry but close this discussion as no progress may be seen. Thank you.

@keroxp keroxp closed this Mar 10, 2019

@bartlomieju

This comment has been minimized.

Copy link
Contributor

bartlomieju commented Mar 10, 2019

@keroxp don't get discouraged, we'll surely settle on some better HTTP implementation. AFAICT those past 2 weeks were focused on improving Deno's core.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.