fix resource race condition

QwikDev · Jun 23, 2024 · e7f732b · e7f732b
1 parent 462f393
commit e7f732b
Show file tree

Hide file tree

Showing 5 changed files with 50 additions and 41 deletions.
diff --git a/packages/qwik/src/core/state/signal.ts b/packages/qwik/src/core/state/signal.ts
@@ -1,7 +1,7 @@
 import { assertEqual } from '../error/assert';
 import { tryGetInvokeContext } from '../use/use-core';
 import { logWarn } from '../util/log';
-import { ComputedEvent, RenderEvent, ResourceEvent } from '../util/markers';
+import { ComputedEvent, RenderEvent } from '../util/markers';
 import { qDev, qSerialize } from '../util/qdev';
 import { isObject } from '../util/types';
 import {
@@ -113,17 +113,12 @@ export class SignalImpl<T> extends SignalBase implements Signal<T> {
         if (invokeCtx.$event$ === RenderEvent) {
           logWarn(
             'State mutation inside render function. Use useTask$() instead.',
-            invokeCtx.$hostElement$
+            String(invokeCtx.$hostElement$)
           );
         } else if (invokeCtx.$event$ === ComputedEvent) {
           logWarn(
             'State mutation inside useComputed$() is an antipattern. Use useTask$() instead',
-            invokeCtx.$hostElement$
-          );
-        } else if (invokeCtx.$event$ === ResourceEvent) {
-          logWarn(
-            'State mutation inside useResource$() is an antipattern. Use useTask$() instead',
-            invokeCtx.$hostElement$
+            String(invokeCtx.$hostElement$)
           );
         }
       }

diff --git a/packages/qwik/src/core/state/store.ts b/packages/qwik/src/core/state/store.ts
@@ -3,7 +3,7 @@ import { assertEqual, assertNumber, assertTrue } from '../error/assert';
 import { QError_immutableProps, qError } from '../error/error';
 import { tryGetInvokeContext } from '../use/use-core';
 import { logError, logWarn } from '../util/log';
-import { ComputedEvent, RenderEvent, ResourceEvent } from '../util/markers';
+import { ComputedEvent, RenderEvent } from '../util/markers';
 import { qDev, qSerialize } from '../util/qdev';
 import { isArray, isObject, isSerializableObject } from '../util/types';
 import {
@@ -233,12 +233,7 @@ export class ReadWriteProxyHandler implements ProxyHandler<TargetType> {
         } else if (invokeCtx.$event$ === ComputedEvent) {
           logWarn(
             'State mutation inside useComputed$() is an antipattern. Use useTask$() instead',
-            invokeCtx.$hostElement$
-          );
-        } else if (invokeCtx.$event$ === ResourceEvent) {
-          logWarn(
-            'State mutation inside useResource$() is an antipattern. Use useTask$() instead',
-            invokeCtx.$hostElement$
+            String(invokeCtx.$hostElement$)
           );
         }
       }

diff --git a/packages/qwik/src/core/use/use-task.ts b/packages/qwik/src/core/use/use-task.ts
@@ -28,7 +28,7 @@ import {
 } from '../state/signal';
 import { implicit$FirstArg } from '../util/implicit_dollar';
 import { logError, logErrorAndStop } from '../util/log';
-import { ComputedEvent, TaskEvent } from '../util/markers';
+import { ComputedEvent, ResourceEvent, TaskEvent } from '../util/markers';
 import { delay, isPromise, safeCall } from '../util/promises';
 import { isFunction, isObject, type ValueOrPromise } from '../util/types';
 import { ChoreType } from '../v2/shared/scheduler';
@@ -689,7 +689,8 @@ export const runResource = <T>(
   task.$flags$ &= ~TaskFlags.DIRTY;
   cleanupTask(task);
 
-  const iCtx = newInvokeContext(container.$locale$, host as fixMeAny, undefined, TaskEvent);
+  const iCtx = newInvokeContext(container.$locale$, host as fixMeAny, undefined, ResourceEvent);
+
   const taskFn = task.$qrl$.getFn(iCtx, () => {
     container.$subsManager$.$clearSub$(task);
   });
@@ -723,31 +724,26 @@ export const runResource = <T>(
   };
 
   const handleError = (reason: unknown) => container.handleError(reason, host);
-  let cleanupFns: (() => void)[] | null = null;
-  const cleanup = (fn: () => void) => {
-    done = true;
-    if (typeof fn == 'function') {
-      if (!cleanupFns) {
-        cleanupFns = [];
-        task.$destroy$ = noSerialize(() => {
-          task.$destroy$ = null;
-          cleanupFns!.forEach((fn) => {
-            try {
-              fn();
-            } catch (err) {
-              handleError(err);
-            }
-          });
-        });
+
+  const cleanups: (() => void)[] = [];
+  task.$destroy$ = noSerialize(() => {
+    cleanups.forEach((fn) => {
+      try {
+        fn();
+      } catch (err) {
+        handleError(err);
       }
-      cleanupFns.push(fn);
-    }
-  };
+    });
+  });
 
   const resourceTarget = unwrapProxy(resource);
   const opts: ResourceCtx<T> = {
     track,
-    cleanup,
+    cleanup(fn) {
+      if (typeof fn === 'function') {
+        cleanups.push(fn);
+      }
+    },
     cache(policy) {
       let milliseconds = 0;
       if (policy === 'immutable') {
@@ -788,6 +784,17 @@ export const runResource = <T>(
     return false;
   };
 
+  /**
+   * Add cleanup to resolve the resource if we are trying to run the same resource again while the
+   * previous one is not resolved yet. The next `runResource` run will call this cleanup
+   */
+  cleanups.push(() => {
+    if (untrack(() => resource.loading) === true) {
+      const value = untrack(() => resource._resolved) as T;
+      setState(true, value);
+    }
+  });
+
   // Execute mutation inside empty invocation
   invoke(iCtx, () => {
     // console.log('RESOURCE.pending: ');
@@ -801,7 +808,7 @@ export const runResource = <T>(
   });
 
   const promise = safeCall(
-    () => taskFn(opts),
+    () => Promise.resolve(taskFn(opts)),
     (value) => {
       setState(true, value);
     },

diff --git a/packages/qwik/src/core/v2/tests/use-resource.spec.tsx b/packages/qwik/src/core/v2/tests/use-resource.spec.tsx
@@ -10,10 +10,8 @@ import {
   useSignal,
 } from '@builder.io/qwik';
 import { describe, expect, it } from 'vitest';
-import { trigger } from '../../../testing/element-fixture';
-import { domRender, ssrRenderToDom } from '../../../testing/rendering.unit-util';
 import '../../../testing/vdom-diff.unit-util';
-import { getTestPlatform } from '@builder.io/qwik/testing';
+import { getTestPlatform, trigger, domRender, ssrRenderToDom } from '@builder.io/qwik/testing';
 
 const debug = false; //true;
 Error.stackTraceLimit = 100;

diff --git a/packages/qwik/src/server/v2-node.ts b/packages/qwik/src/server/v2-node.ts
@@ -85,6 +85,20 @@ export class SsrNode implements ISsrNode {
       mapApp_remove(this.attrs, name, 0);
     }
   }
+
+  toString(): string {
+    let stringifiedAttrs = '';
+    for (let i = 0; i < this.attrs.length; i += 2) {
+      const key = this.attrs[i];
+      const value = this.attrs[i + 1];
+      stringifiedAttrs += `${key}=`;
+      stringifiedAttrs += `${typeof value === 'string' || typeof value === 'number' ? JSON.stringify(value) : '*'}`;
+      if (i < this.attrs.length - 2) {
+        stringifiedAttrs += ', ';
+      }
+    }
+    return `SSRNode [<${this.id}> ${stringifiedAttrs}]`;
+  }
 }
 
 export type SsrNodeType = 1 | 3 | 9 | 11;