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

Fix infinite loop in Suspense error handling on the server #1649

Merged
merged 1 commit into from
Mar 26, 2023
Merged

Fix infinite loop in Suspense error handling on the server #1649

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 8 additions & 9 deletions packages/solid/src/server/rendering.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -552,7 +552,6 @@ export function SuspenseList(props: {

export function Suspense(props: { fallback?: string; children: string }) {
let done: undefined | ((html?: string, error?: any) => boolean);
let clean: any;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

clean wasn't being used anywhere anymore. maybe that's a mistake? not sure what it did

const ctx = sharedConfig.context!;
const id = ctx.id + ctx.count;
const o = createOwner();
Expand All @@ -569,8 +568,8 @@ export function Suspense(props: { fallback?: string; children: string }) {
});
function runSuspense() {
setHydrateContext({ ...ctx, count: 0 });
o && cleanNode(o);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

o is always present. before 1.6.13 clean was checked here

Copy link
Member

Choose a reason for hiding this comment

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

Are we sure we can just remove it? I hoisted it up for sure and that caused the issue, but it was always there. I guess I can retest the original React Query scenario.

Copy link
Contributor Author

@thetarnav thetarnav Mar 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

pre 1.6.13 the current Owner would get assigned as o so it could be null, now we are always creating a new owner so the condition is pointless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, github is showing the preview a bit weirdly. I didn't remove cleanNode(o), just o &&

Copy link
Member

Choose a reason for hiding this comment

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

Oh I get it. Yeah I was looking at the wrong part. I guess the trickiest part is I change this mechanism completely in 1.7. It's going to be a bit interesting to figure out how to merge this back into next. It looks like I need to do another 1.6 release given the stuff I've broken though so I will go with this for now.

return runWithOwner(o!, () => {
cleanNode(o);
return runWithOwner(o, () => {
return createComponent(SuspenseContext.Provider, {
value,
get children() {
Expand All @@ -584,14 +583,14 @@ export function Suspense(props: { fallback?: string; children: string }) {
// never suspended
if (suspenseComplete(value)) return res;

onError(err => {
if (!done || !done(undefined, err)) {
if (o)
runWithOwner(o.owner!, () => {
runWithOwner(o, () => {
onError(err => {
if (!done || !done(undefined, err)) {
runWithOwner(o.owner, () => {
throw err;
});
else throw err;
}
}
});
});
done = ctx.async ? ctx.registerFragment(id) : undefined;
if (ctx.async) {
Expand Down