Skip to content

fix(proxy): better error when upstream proxy fails#746

Merged
pi0 merged 10 commits intomainfrom
fix/proxy-500
Jun 19, 2024
Merged

fix(proxy): better error when upstream proxy fails#746
pi0 merged 10 commits intomainfrom
fix/proxy-500

Conversation

@TheAlexLichter
Copy link
Copy Markdown
Contributor

@TheAlexLichter TheAlexLichter commented May 5, 2024

🔗 Linked issue

#722

❓ Type of change

  • 📖 Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • 👌 Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

This PR should ensure that proxyRequests fail gracefully instead of leading to an unhandled node error crashing the server.

📝 Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages bot commented May 5, 2024

Deploying unjs-h3 with  Cloudflare Pages  Cloudflare Pages

Latest commit: dc9f019
Status: ✅  Deploy successful!
Preview URL: https://cfe56f7c.unjs-h3.pages.dev
Branch Preview URL: https://fix-proxy-500.unjs-h3.pages.dev

View logs

@TheAlexLichter TheAlexLichter marked this pull request as ready for review May 5, 2024 13:57
@TheAlexLichter TheAlexLichter changed the title test: add failing proxy test fix: graceful fail for proxy May 5, 2024
Comment thread src/utils/proxy.ts Outdated
@pi0 pi0 changed the title fix: graceful fail for proxy fix(proxy): better error when upstream proxy fails May 7, 2024
@TheAlexLichter TheAlexLichter self-assigned this May 7, 2024
@TheAlexLichter TheAlexLichter marked this pull request as draft May 7, 2024 13:57
@TheAlexLichter TheAlexLichter marked this pull request as ready for review May 8, 2024 14:46
@TheAlexLichter
Copy link
Copy Markdown
Contributor Author

Updated to sendProxy.

I think only we can assume only proxy had been failed and perhaps we can double check if there is not already set statusCode and message on error (that currently is being ingored)

You mean an existing one in event.node.statusCode / statusMessage or in the error directly? If the latter, which values should I check?

Copy link
Copy Markdown
Member

@pi0 pi0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks ❤️ (updated impl to still throw error to allow hooking into it but with createError)

@pi0 pi0 merged commit 746a6c5 into main Jun 19, 2024
@pi0 pi0 deleted the fix/proxy-500 branch June 19, 2024 00:03
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.

2 participants