Skip to content

Conversation

nyannyacha
Copy link
Contributor

What kind of change does this PR introduce?

Bug fix

Description

For a promise to a WritableStream that streams the body of an inbound request to a user worker, the promise might be rejected in some cases.

Previously, if this promise was rejected, an exception to this rejection would be propagated to the caller.

let [sent, res] = await Promise.allSettled([reqBodyPromise, resPromise]);

The promise res, which gets the response from the user worker, and the promise sent, which streams the request body to the user worker, are bound by Promise.allSettled, so it will not return until all of the passed Promises have performed their actions.

However, propagating exceptions for these sent is turns out to be undesirable.

This is because the user worker may consume the request object abnormally, or op_user_worker_fetch_send may drop the channel object in the body under the request object, causing the promise to be rejected.

Therefore, we should always expose only the result for res to the caller.

Fixes #366

@nyannyacha nyannyacha marked this pull request as ready for review June 24, 2024 03:13
@nyannyacha nyannyacha requested a review from laktek June 24, 2024 04:12
Copy link
Contributor

@laktek laktek left a comment

Choose a reason for hiding this comment

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

LGTM! should we add a test for this?

@nyannyacha nyannyacha merged commit 5657032 into supabase:main Jun 24, 2024
@nyannyacha nyannyacha deleted the fix-issue-366 branch June 24, 2024 06:03
Copy link

🎉 This PR is included in version 1.54.7 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

TypeError request body receiver not connected (channel closed)
2 participants