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

feat(std/http): add HTTP server "error" event #703

Closed
wants to merge 8 commits into from

Conversation

lideming
Copy link
Contributor

@lideming lideming commented Feb 3, 2021

This PR allows the user to handle/log the error from the listener and the request reader.
(Moved from denoland/deno#8554)

Before:
The HTTP server rethrows the error from the listener if the error type is unknown, and causes an uncaught error. Also, it ignores all errors while reading requests.

After:
The Server class is now inherited from EventTarget. It will pass the error to the error event. The user can handle it after adding an event listener to the server. By default, the server keeps running after encountering an error.

Related issue: denoland/deno#8499 (not closing it because I don't know where did the ENOTCONN come from)

Usage:

const server = serve({ port: 8000 });
server.addEventListener('error', (evt) => {
  if (evt.origin == "request") {
    console.warn("Error reading request", evt.connection, evt.error);
    // The server will send "400 Bad Request" and close the connection
    // unless `evt.preventDefault()` is called.
  } else if (evt.origin == "listener") {
    console.warn("Error accepting connection", evt.error);
    // The server will continue to accept connections
    // unless `evt.preventDefault()` is called.
  }
})
for await (const req of server) {
  req.respond({ body: "Hello World\n" });
}

This PR also make file_server print errors from the http server.

Something to discuss:

  1. Is it okay to have Server extends EventTarget to implement events? Should we implement it in another way? While I am guessing it will hardly break something, should it be considered a BREAKING change?
  2. server.addEventListener("error", (evt) => { ... }) then check evt.origin == "request", or just server.addEventListener("request-error", ...)?

@CLAassistant
Copy link

CLAassistant commented Feb 3, 2021

CLA assistant check
All committers have signed the CLA.

@timonson
Copy link
Contributor

Hi @lideming would you mind telling me if this PR would fix this issue?

@lideming
Copy link
Contributor Author

@timonson this PR catches errors from the listener and request reader, but not from ServerRequest.respond.

@bartlomieju
Copy link
Member

@cmorten do you have an opinion on this PR?

@cmorten
Copy link
Contributor

cmorten commented Sep 6, 2021

It's a curious one - certainly we should look to re-invest future efforts in the new "native" abstraction over the old JS Server implementation.

There is no precedent for error eventing (nor error callback or sim. etc.) in the golang std lib from which the RFC / new impl. was inspired - it instead opts for an ErrorLog logger.

// ErrorLog specifies an optional logger for errors accepting
// connections, unexpected behavior from handlers, and
// underlying FileSystem errors.
// If nil, logging is done via the log package's standard logger.
ErrorLog *log.Logger

REFs:

Looking to JS frameworks, there is prior art in Oak (REF: https://github.com/oakserver/oak) for successful incorporation of eventing into the server (application), indeed it extends EventTarget, just as this PR suggests, supporting the likes of listen, error, and abort events.

Equally, the likes of Express (REF: https://github.com/expressjs/express) also support events via the Node events built-in EventEmitter (though has limited use), and Node's net.Server also extends EventEmitter with a plethera of events such as listening and error.

Because we have adopted the golang style in most cases to gracefully handle the majority of errors (closing connections as required etc.), there is little to nothing that a consumer can "hook" into if they do want act upon some exception. Though I suspect the majority of error handling can be done in the consumer's handler itself, I could foresee them wanting to know when say accepting a connection has failed for one reason or another etc. - things like APM / instrumentation come to mind, and certainly if there have been a cascade of tls handshake issues it would be good to allow the consumer to gets eyes on that if they desire for debug.

Using the Web EventTarget makes sense (over say Node's rolled EventEmitter) in the aim to utilise where can Web APIs (certainly should one wish to mirror https://stackblitz.com/ for Deno, I'd hope the usage of Web APIs would make the effort somewhat less than with Node!). An obvious alternative would be to provide an onError like handler to the server on instantiation, but I feel eventing is far superior in this instance with the flexibility to add and remove listeners as the server is running.

R.E.

server.addEventListener("error", (evt) => { ... }) then check evt.origin == "request", or just server.addEventListener("request-error", ...)?

One benefit of an EventTarget is the use of event namespacing to be declarative in style when events are broadcasted to listeners - I would suggest we look to avoid overloading a single event namespace where there is a clear distinction as it is generally more scalable to have multiple well scope listeners over a single listener with a large switch statement. We should utilise the fact that EventTarget can save consumers on branching logic. That said, we needn't over-segment unnecessarily - for guidance I believe Node's http.Server has only two forms of "error" event: error and clientError.

REFs:

A server's behaviour is generally divided into (1) listening/accepting connections, and (2) HTTP over said connections, so in agreement with this PR, perhaps we could consider connError and requestError (or sim. nomenclature) and divide errors between those events… though as I write, do wonder if just error would suffice (despite everything just said 😅).

One thing would be good to ensure is that with the greater machinary involved (EventTarget or sim. + firing errors) that we don't degrade/regress the golden-path performance of the server too much. Certainly errors will be slower, but exceptions always are - finding / keeping the balance between developer experience / tooling and perf feels like an important factor in this part of the std lib. Despite discarding it earlier, simply introducing a callback handler (which I guess is similar in feel to golang's ErrorLogger, only not specifically a logger) would potentially be lighter touch and have less of a performance impact? Only PoC + benchmarking could tell 😄

@bartlomieju
Copy link
Member

@cmorten thanks for in-depth write up. Especially the last paragraph about performance impact is standing out; that said I want to know what @kitsonk thinks about it. If oak successfully uses standard DOM event listeners maybe it might be worth exploring providing that functionality in std/http?

Additionally I believe it would nicely integrate with Deploy.

@cmorten
Copy link
Contributor

cmorten commented Sep 6, 2021

@cmorten thanks for in-depth write up. Especially the last paragraph about performance impact is standing out; that said I want to know what @kitsonk thinks about it. If oak successfully uses standard DOM event listeners maybe it might be worth exploring providing that functionality in std/http?

I suspect even with a perf hit it is worth doing - W.r.t Oak, I’d feel like we’d have got std/http right when the low level of something like oak could just be swapped out for the std lib, framework authors can then spend their time adding value at higher levels over dealing with the nitty gritty around the Deno APIs. Indeed there is already quite some overlap between the std/http server and Oak (especially as used as inspiration, and latter contributed the close logic back!). Be great to hear @kitsonk’s thoughts.

Additionally I believe it would nicely integrate with Deploy.

Agreed, now there is support (REF: https://deno.com/blog/deploy-beta2#deno.listen-and-deno.servehttp) it would be great to keep deploy in mind 😄

@bartlomieju
Copy link
Member

This PR is quite stale, there were many changes to http/ module since it was opened. @lideming if you are still determined to get this change landed, please open a new one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants