From 6a13b170a2b1e74e32ffa6293b0b39739ab99ea6 Mon Sep 17 00:00:00 2001 From: tigt Date: Wed, 8 Apr 2020 02:20:53 -0400 Subject: [PATCH 1/3] WIP: initial thoughts on stream error handling --- ...000-surface-async-stream-errors-to-http.md | 231 ++++++++++++++++++ 1 file changed, 231 insertions(+) create mode 100644 proposals/0000-surface-async-stream-errors-to-http.md diff --git a/proposals/0000-surface-async-stream-errors-to-http.md b/proposals/0000-surface-async-stream-errors-to-http.md new file mode 100644 index 0000000..6a7bdaf --- /dev/null +++ b/proposals/0000-surface-async-stream-errors-to-http.md @@ -0,0 +1,231 @@ +# Surfacing Async Stream Errors to HTTP + +Michael Rawlings +Marko doesn't do anything specific at the http level since right now the api is rendering to a stream (usually an http response, but could be something like an fs write stream). For errors that aren't handled by ``'s `<@catch>`, an error event is emitted on the stream. But for caught errors, I'm not sure what the best course of action is here. Should marko handle closing the stream or should it emit a caught-error event so that the application can do the right thing based on the protocol? + +It's a little strange too because in the case of a caught error we want to finish rendering the page and send it, but we may still want to signal to the browser that something went wrong. + +Michael Rawlings + + +Taylor Hunt +and I guess Marko's whole reason for being is efficiently rendering HTML +which is why you probably shouldn't use it for, say, emitting `text/event-stream` or WebSockets + +## Intro + +> A _short_ explanation of the proposal. + +## Motivation + +Marko already has a way to tell _humans_ that the promise for an `` rejected: the `<@catch>` block. Unfortunately, for machines, the original HTTP status code is how they determine the error state of a response — and the HTTP headers are already sent by the time an `` errors. + +Say we have Marko render the page’s `` and the site logo/navigation/searchbar while the Node server calls a backend API to populate the page content: + +```marko + + + + + + + + + + + + + <@then|response|> + ${response.body} + + <@catch|err|> + Oh no, the content API is down again + + + + + + + +``` + +This is great for the user because their browser gets a head start downloading assets linked in the `` and displaying the site header while the network request to the content API is underway. + +However, if the `contentFetchRequest` fails for any of the myriad reasons computers are terrible, the page will show an error message instead of its real content — but as far as the HTTP layer knows, the response completed successfully. + +No machine-readable indicator that the streamed response contains erroneous content causes some thorny problems: + +- An HTTP cache may store the erroneous content and reuse it, causing users to see the error for longer than they otherwise would + +- A search engine will index the erroneous content, since they received no sign they should try again or discard the response as invalid + +- HTTP-level tools (debuggers, curl, spiders, etc.) will report the response as successful, even when it wasn’t + +Research into HTTP/1.1’s chunked `Transfer-Encoding` and HTTP/2’s stream error handling found that they both have a standardized way of indicating a dynamically-streamed response failed to complete successfully: + +
+
HTTP/1.1 Transfer-Encoding: chunked
+
IETF Draft: HTTP/1.1 Messaging §8 Handling Incomplete Messages
+

If a chunked response doesn’t terminate with the zero-length end chunk, the client must assume that the response was incomplete — which at the very least, means a cache should double-check with the server before reusing the stored incomplete response.

+ +
HTTP/2 Streams
+
RFC 7540 §5.4.2 Stream Error Handling
+

An HTTP/2 stream can signal an application error by sending a RST_STREAM frame with an error code of 0x2 INTERNAL_ERROR.

+
+ +This proposal explores how Marko should surface errors from server components to the HTTP layer, so the response can properly signal an error with one of the above methods. + +## Guide-level explanation + +Developers should be able to tell Marko how important an async error is: + +1. Our example from the Motivation section is as serious as it gets — you might even want to redirect to a completely different error page +2. Smaller partials in a larger page may still want to tell proxies and search engines that the response is incomplete, but the user might find value in the rest of the page around the `<@error>` +3. You might not care at all for trivial parts of the page, like pulling weather data to change the background image or something + +> Explain the proposal as if it was already implemented and you are now teaching it to another Marko developer. That generally means: +> +> - Explaining the feature largely in terms of examples. +> - What names and terminology work best for these concepts and why? Is it a continuation of existing Marko patterns, or a wholly new one? +> - Explaining how Marko developers should _think_ about the feature, and how it should impact the way they use Marko. It should explain the impact as concretely as possible. +> - If applicable, describe the differences between teaching this to existing Marko developers and new Marko developers. + +> For implementation-oriented proposals, this section should focus on how contributors should think about the change, and give examples of its concrete impact. For policy proposals, this section should provide an example-driven introduction to the policy, and explain its impact in concrete terms. + +> Would the acceptance of this proposal mean the Marko docs must be re-organized or altered? + +## Reference-level explanation + +> This is the technical portion of the proposal. Explain the design in sufficient detail that: +> +> - Its interaction with other features/tools is clear. +> - It is reasonably clear how the feature would be implemented. +> - Corner cases are dissected by example. +> - The section should return to the examples given in the previous section, and explain more fully how the detailed proposal makes those examples work. + +It is out of scope for this mechanism to surface anything other than runtime errors — mostly because that’s the only semantic we can surface through either version of HTTP. It would be wrong to use this for a 404 page, for example. + +### Trailers + +We should send error information in HTTP trailers if any clients would benefit. [Some browsers have traditionally discarded almost all trailers other than `Server-Timing`](https://www.fastly.com/blog/supercharging-server-timing-http-trailers), but other browsers, tools like `curl`, search engines, proxy caches, and debuggers may benefit. For example, an updated `Cache-Control`, `Retry-After`, or even `Refresh`ing to an error page at a new URL if the problem was serious enough. + +Even for stricter browsers, error information inside `Server-Timing` could help developers, as many monitoring tools track, expose, and alert information from this header. + +``` +Server-Timing: markoAsyncErr; dur=87.1; desc="${errorInfo}" +``` + +What information should be exposed this way? + +- The `dur` field is standardized as indicating how far along the event happened in the request +- The `desc` field is any string the developer wishes +- Multiple Server-Timing events are allowed with the same name, so Marko could emit it repeatedly in the case of multiple async failures, with `desc` disambiguating what happened and where + +However, note that over HTTP/1.1, trailers can only be appended _after_ the zero-length terminator chunk. Hmm. + +### HTTP/1.1 considerations + +To avoid performance problems with `keepalive` connections, it is probably not wise to explicitly call `.close()` — instead, the streamed HTML response should finish without ever sending the zero-length chunk terminator. + +But does that mean that kept-alive connection never gets reused by the client, therefore blocking other resources from getting sent over it? + +Maybe we should `.close()` the connection only once we know we’re done rendering the page. + +The spec also indicates a `Transfer-Encoding` decode error also marks the message as incomplete; a chunk with a _negative_ integer length might trigger that behavior? Or any other data that isn’t a positive hex integer. + +A chunk that’s displaying the contents of a `<@catch>` error message may also encode error information as [chunk extensions](https://tools.ietf.org/html/rfc7230#section-4.1.1). I wonder if there’s any standards or _de facto_ implementations that leverage these? + +What a headache. At least HTTP/2 has a much more explicit signal for errors during a stream. + +### HTTP/2 considerations + +Developers could signal that even though an error occurred, the request is safe to retry. [The HTTP/2 spec provides two methods to indicate which streamed requests are safe to retry](https://tools.ietf.org/html/rfc7540#section-8.1.4): a `RST_STREAM` frame with an error code of `REFUSED_STREAM`, or a `GOAWAY` frame indicating which stream IDs are safe to retry. + +However, those signals indicate a retry is safe even if the request semantics were not idempotent (like a `POST` request), so they may not be useful by the time the request handling makes it to the Marko layer: + +> A server MUST NOT indicate that a stream has not been processed unless it can guarantee that fact. If frames that are on a stream are passed to the application layer for any stream, then `REFUSED_STREAM` MUST NOT be used for that stream, and a `GOAWAY` frame MUST include a stream identifier that is greater than or equal to the given stream identifier. + +Still, we might provide a way for Marko authors to signal this safe-to-retry information — the application may be allowed to reason about errors within its own layer. + +### Signaling within Node + +Node’s `http` and `http2` modules are probably capable of emitting error state as the standards require, but it’s not 100% clear how to transparently work with them in ways that the greater ecosystem would expect. + +For example, how does Express react to a `http` stream closing without the zero-length terminator? + +Hopefully, Node already does The Right Thing when these modules’ streams produce the protocol-specific error signal, and the ecosystem has had time to discover and handle that occasion. (If not, Node might appreciate a pull request.) More research needed. + +Presumably-helpful parts of the API: + +- https://nodejs.org/api/net.html#net_socket_end_data_encoding_callback +- https://nodejs.org/api/net.html#net_socket_setkeepalive_enable_initialdelay +- https://nodejs.org/api/http.html#http_request_abort +- https://nodejs.org/api/http.html#http_event_clienterror +- `http2session.destroy([error][, code])` +- `http2session.goaway([code[, lastStreamID[, opaqueData]]])` +- https://nodejs.org/api/http2.html#http2_destruction +- `http2stream.close(code[, callback])` +- `response.addTrailers(headers)` + +### HTTP ecosystem considerations + +It should be possible for developers to selectively turn this error-signaling behavior off, in the case of misbehaving proxies or known-bad clients. + +Research needed into how common reverse proxies or load balancers, such as nginx, handle error-signaled HTTP streams. Real-world examples of how that can happen: https://rhodesmill.org/brandon/2013/chunked-wsgi/ (including the comment at the very bottom) + +https://rijulaggarwal.wordpress.com/2018/01/10/atmosphere-long-polling-on-nginx-chunked-encoding-error/ + +In the common case of the backend speaking HTTP/1.1 to a reverse proxy/front-end terminator/CDN/etc. that translates to HTTP/2 for browsers, what can be done? + +## Migration strategy + +> Is this proposal backwards incompatible? Does this proposal replace an existing feature/pattern? If so, can we safely and automatically migrate existing apps/components to this proposal? How? + +> If applicable, provide sample error messages, deprecation warnings, or migration guidance. + +## Drawbacks + +So far, Marko is totally agnostic as to what kind of stream it’s outputting to. You could use it to stream to a file, or something really unexpected like an FTP stream transmission or a chat protocol. + +It’s possible the server runtime might expose a different method to avoid breaking such cases, like `template.streamToHttp()`. + +However, there’s already a lot about Marko’s server rendering that only makes sense delivered over HTTP. For example, if you were rendering to a file to be served later, you’d want to buffer the component init scripts and you’d never want `client-reorder` enabled. + +## Alternatives + +The impact of not doing this would be no change; we would continue exposing Marko authors to the risks mentioned in the Motivation section. + +In the past, browsers other than Internet Explorer accepted `multipart/x-mixed-replace` responses to be loaded in `target="_top"`, and that could potentially be used to swap to a a new copy of the response-in-progress but with error-indicating headers, or potentially even a replacement error page. However, modern browsers have quietly dropped support for `multipart/x-mixed-replace` in the top browsing context. + +It _might_ be possible to write ``. It’s not _supposed_ to be outside the ``, but I still like it better than how Rails creates a mid-stream error redirect with an inline `