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.

Sign up for GitHub

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

Jump to bottom

Node adapter error handling in http stream: throwing async iterable shouldn't result in last HTTP/1.1 chunk being zero-length #8714

Closed
mb21 opened this issue Sep 30, 2023 · 8 comments · Fixed by #9908
Closed

Node adapter error handling in http stream: throwing async iterable shouldn't result in last HTTP/1.1 chunk being zero-length #8714

mb21 opened this issue Sep 30, 2023 · 8 comments · Fixed by #9908
Assignees
@lilnasy
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) pkg: node Related to Node adapter (scope)

Comments

@mb21
Copy link

mb21 commented Sep 30, 2023 •
edited
Loading

Astro Info

Astro                    v3.2.0
Node                     v18.18.0
System                   Linux (x64)
Package Manager          npm
Output                   server
Adapter                  @astrojs/node
Integrations             none

Describe the Bug

It's great that Astro handles JS iterables in its templates and converts them to an HTTP stream, as mentioned in the docs.

However, async iterables can throw/reject, and that case isn't handled very gracefully. (Of course, the 200 status-headers have already been sent, so that's not an option.) The Astro Node adapter only supports HTTP/1.1 (if I understand correctly), so there it's actually fairly clear what to do: simply not terminate with the zero-length end chunk. Because (from marko-js/community#1):

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.

But Astro currently returns a zero-length chunk.

To reproduce:

---
function * generator () {
  yield 1
  throw Error('ohnoes')
}
---
<body>
  {generator()}
</body>

and run npm run build && npm run preview, then in another terminal curl --raw -i http://localhost:4321. This prints.

HTTP/1.1 200 OK
Transfer-Encoding: chunked
[...]
<body>
1
1
15
Internal server error
0

What's the expected result?

HTTP/1.1 200 OK
Transfer-Encoding: chunked
[...]
<body>
1
1
15
Internal server error

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-v33sgm?file=src%2Fpages%2Findex.astro

(Note that the curl built into stackblitz somehow doesn't have a --raw option, so you need to download the example.)
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Sep 30, 2023
@mb21 mb21 changed the title Node adapter error handling in http stream: throwing async iterable shouldn't result in zero-length-terminated HTTP/1.1 chunk Node adapter error handling in http stream: throwing async iterable shouldn't result in last HTTP/1.1 chunk being zero-length Sep 30, 2023
@matthewp matthewp added pkg: node Related to Node adapter (scope) - P3: minor bug An edge case that only affects very specific usage (priority) and removed needs triage Issue needs to be triaged labels Oct 2, 2023
@matthewp
Copy link
Contributor

matthewp commented Oct 2, 2023

Thanks @mb21, sounds like you understand the topic quite well and we would very much appreciate a PR to resolve this.

@mb21
Copy link
Author

mb21 commented Oct 3, 2023 •
edited
Loading

I mostly collected info from that linked marko issue ;-) Maybe I'll have time to take a stab at it later, but I haven't dug much in the astro code-base yet... would also be important to know whether the node server is planned to have HTTP/2 support anytime soon or not...

We currently catch the throwing async iterator in our code base, put an error message at that place in the HTML, and continue to render the rest of the HTML. For that case, optimally we would have a way to signal CDNs and other caches about that, i.e. again end with a non-zero-lenght HTTP/1.1 chunk. Possible interface would be a Astro.response.markStreamError() function or similar...

Note: the code that currently handles this can be found here.

@lilnasy lilnasy self-assigned this Jan 5, 2024
@lilnasy
Copy link
Contributor

lilnasy commented Jan 31, 2024

@mb21 Node servers are typically run behind reverse proxies which would handle HTTPS, HTTP/2, and even HTTP/3. Would there be value to directly supporting HTTP/2?

@mb21
Copy link
Author

mb21 commented Jan 31, 2024

Node servers are typically run behind reverse proxies which would handle HTTPS, HTTP/2, and even HTTP/3. Would there be value to directly supporting HTTP/2?

I think you're right that there is little benefit. This SO answer mentions "HTTP/2 server push" and some security benefits though... 🤷

@lilnasy
Copy link
Contributor

lilnasy commented Jan 31, 2024

Do you know if http/2 would mean better behavior for mid-stream failures?

@mb21
Copy link
Author

mb21 commented Jan 31, 2024

@lilnasy no, the way to signal a mid-stream failure is just different.

@lilnasy lilnasy mentioned this issue Jan 31, 2024
Merged
@lilnasy
Copy link
Contributor

lilnasy commented Jan 31, 2024

Server push is deprecated. If there is a good use-case for native http/2, feel free to open an issue - I'm pretty sure it can be added very easily.

@mb21
Copy link
Author

mb21 commented Feb 1, 2024

Nice work @lilnasy !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lilnasy lilnasy

Labels
- P3: minor bug An edge case that only affects very specific usage (priority) pkg: node Related to Node adapter (scope)
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(NodeApp): end with "Internal server error" on mid-stream error lilnasy/astro
3 participants
@mb21 @matthewp @lilnasy