Add serveGuarded and serveRequestsGuarded API

[annagrin]

serveRequests does not always wrap listening to http requests in a runZoneGuarded wrap.

This leads to hard to debug async exceptions later if something up its call chain adds its own runZoneGuarded wrapper - in that case exceptions stop being caught and ignored inside serveRequests and fall to the next root zone exception handler, which unexpectedly changes the logic of the calling code.

Note that adding such top level handler will change behavior of all serveRequests calls, including the ones coming from imported libraries, which makes fixing those new issues particularly complicated.

This change adds two new APIs, serveGuarded and serveRequestsGuarded, that alleviate the issue by always serving requests in an error zone and enabling the user to provide an error handler. A simple print-and-continue error handler is used by default.

In addition, updated the message on the default error handler to reflect source of the errors.

Closes: https://github.com/dart-lang/shelf/issues/202

[kevmoo]

To pile on: wonder if it would be good to give references to these functions from the existing ones – so folks know there are other options?

[natebosch]

wonder if it would be good to give references to these functions from the existing ones – so folks know there are other options?

I'd like to standardize onto the new API and drop the old one.

[annagrin]

wonder if it would be good to give references to these functions from the existing ones – so folks know there are other options?

I'd like to standardize onto the new API and drop the old one.

Should I remove the old API in this PR, or is there a deprecation procedure?

[jakemac53]

Should I remove the old API in this PR, or is there a deprecation procedure?

You can add an @Deprecated('Use serveGuarded') annotation.

[annagrin]

@natebosch @kevmoo @jakemac53 I think this PR is going towards revisiting some API and looking at their usages, I would be happy if one of you decides to take over:)

[natebosch]

I dug a bit deeper and remembered the point at which this became more complex enough for me to drop it last time I looked.

We should also fix the other places that errors can surface as direct writes to stderr:

https://github.com/dart-lang/shelf/blob/4af758ac6577037dd55107a0e6571be442d22407/lib/shelf_io.dart#L87-L108

These calls to _logTopLevelError should instead go to the caller's error handler.

Probably the easiest way to handle this is to also base this new API on a separate copy of the implementation with improved semantics, and then migrate uses one by one.

All of this is to say - I think this package needs a good deal of attention and I'm not sure a half-measure is really worthwhile in the short term, given that we have existing workarounds.

I'll spend a little more time scoping out some potential improvements and see if it makes sense to push on them.

[jakemac53]

I am fine with just doing the workarounds for now where we need and doing a more general rewrite here.

[annagrin]

I am checking in workarounds in a few of our libraries that are affecting DDR.

A few things that can be useful for the future library:

• some configuration that the user can pass to a library call that eventually starts a server
  • including error handler(s)
• if possible to split errors into categories - for example, some of the errors are now converted into 404 or 500 responses, but some are falling through, like socket exceptions. If we pass them all to the user-provided error handler, the user would need to know how to handle them.
• server should not throw exceptions when overloaded (at least non-recoverable ones) - currently we have to limit all clients max connections, otherwise we get async exceptions about broken pipes, http exceptions, "connection reset by peer" etc. Should the server give an error response instead? Or maybe the default http client needs a max connections limit set?

[kevmoo]

@annagrin – any plans here?

[natebosch]

I had started thinking about the design here and we decided to hold off on this PR, but then I never finalized a plan. I had some work towards a cleaner version of serveRequests... I'll see if I can track that down.

[annagrin]

@kevmoo I will defer the proper fix to the active library developers (@natebosch I presume). To fix the issues caused by this, I had to wrap the serveRequests call in an error zone in all affected places (there are probably more, but the ones I found were in dwds, webdev, devtools server, ddr, and if I remember correctly build_daemon). The SocketException crash rate dropped 10x but they are still happening to approx 1.5% of our sessions. I might have missed some other cases, which may or may not relate to this issue.

[mosuem]

Closing this, as it is stale and the files are out of date. Feel free to reopen in case you still want to land it!