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.

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

Memory Leak on prettyDOM calls while waiting for expect.element #7139

Closed
6 tasks done
tsirlucas opened this issue Dec 27, 2024 · 2 comments
Closed
6 tasks done

Memory Leak on prettyDOM calls while waiting for expect.element #7139

tsirlucas opened this issue Dec 27, 2024 · 2 comments
Labels
feat: browser Issues and PRs related to the browser runner p4-important Violate documented behavior or significantly improves performance (priority)

Comments

@tsirlucas
Copy link
Contributor

Describe the bug

While waiting for expect.element to resolve, prettyDOM will be called on every attempt and its going to send the whole body as dom parameter. This makes memory usage go wild and at some point it goes OOM. Can confirm removing prettyDom usage in following line fixes the issue:

const error = new Error(`Cannot find element with locator: ${asLocator('javascript', selector)}\n\n${prettyDOM(container)}`)

IDK how Im the first person to notice this bug... I wonder if Im missing a config param or something.

Reproduction

Had to reproduce it on private codebase so I cant really share code here.

image image image

After removing the prettyDOM call, all tests pass successfully:

image

System Info

System:
    OS: macOS 15.1.1
    CPU: (14) arm64 Apple M3 Max
    Memory: 8.26 GB / 36.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.12.2 - ~/.nvm/versions/node/v20.12.2/bin/node
    Yarn: 4.5.1 - ~/.nvm/versions/node/v20.12.2/bin/yarn
    npm: 10.5.0 - ~/.nvm/versions/node/v20.12.2/bin/npm
  Browsers:
    Chrome: 131.0.6778.205
    Safari: 18.1.1

Used Package Manager

yarn

Validations

@sheremet-va sheremet-va added p4-important Violate documented behavior or significantly improves performance (priority) and removed pending triage labels Dec 27, 2024
@sheremet-va sheremet-va added the feat: browser Issues and PRs related to the browser runner label Dec 27, 2024
@sheremet-va sheremet-va moved this to Approved in Team Board Dec 27, 2024
@tsirlucas
Copy link
Contributor Author

tsirlucas commented Dec 27, 2024

@sheremet-va if you havent started working on it yet, do you mind sharing the approach youd like to take here? i have some free time today and would be nice to have a contribution to the project

EDIT: I think we have a few options

  • Flag the poll call with an extra parameter to tell when its the last call before timeout and only call prettyDOM in that situation (Im not sure about this one cause it will require some major changes and/or facing possible racing conditions here)

  • Use .query() instead of .element() here and throw a custom error without calling prettyDOM (also not a fan here since it will mean dropping prettyDOM on expect.element() calls)

  • I suspect that the memory leak is actually caused by main thread being overwhelmed with the amount of prettyDOM calls, so GC is not able to do its job properly. Maybe moving that code to a web worker or giving some time to breath with old good setTimeout(fn, 0)?

Let me know if im missing something

EDIT 2: I dont think option 3 is easily feasible because of the sync nature of how prettyDOM is used in that scenario.

In other hand, I gave a shot to option 1 locally and it works well. Instead of flagging the last attempt, I just try one last time before rejecting the promise here.

Copy-pasting the hardcoded solution I have locally so you can check:

packages/vitest/src/integrations/chai/poll.ts

 return function(...args) {
     const STACK_TRACE_ERROR = new Error("STACK_TRACE_ERROR");
     const promise = () => new Promise((resolve, reject) => {
       let intervalId;
       let timeoutId;
       let lastError;
       const { setTimeout, clearTimeout } = getSafeTimers();
       const check = async () => {
           try {
             chai$1.util.flag(assertion, "_name", key);
             const obj = await fn();
             chai$1.util.flag(assertion, "object", obj);
             resolve(await assertionFunction.call(assertion, ...args));
             clearTimeout(intervalId);
             clearTimeout(timeoutId);
           } catch (err) {
             lastError = err;
-            intervalId = setTimeout(check, interval);
+            if (!chai$1.util.flag(assertion, "_isLastPollAttempt")) {
+              intervalId = setTimeout(check, interval);
+            }
           }
         };
       timeoutId = setTimeout(async () => {
         clearTimeout(intervalId);
+        chai$1.util.flag(assertion, "_isLastPollAttempt", true);
+        await check();
         reject(
           copyStackTrace$1(
             new Error(`Matcher did not succeed in ${timeout}ms`, {
               cause: lastError
             }),
             STACK_TRACE_ERROR
           )
         );
       }, timeout);
       check();
     });
 }

packages/browser/src/client/tester/expect-element.ts

 return expect.poll(function element() {
     if (elementOrLocator instanceof Element || elementOrLocator == null) {
       return elementOrLocator;
     }
     chai.util.flag(this, "_poll.element", true);
     const isNot = chai.util.flag(this, "negate");
     const name = chai.util.flag(this, "_name");
+    const isLatPollAttempt = chai.util.flag(this, "_isLastPollAttempt");
     if (isNot && name === "toBeInTheDocument") {
       return elementOrLocator.query();
     }

+    if (isLatPollAttempt) {
+      return elementOrLocator.element();
+    }
+
+    const result = elementOrLocator.query();
+
+    if (!result) {
+      throw new Error(`Cannot find element with locator: ${JSON.stringify(elementOrLocator)}`);
+    }
+
+    return result;
   }, options);

@sheremet-va
Copy link
Member

  • Flag the poll call with an extra parameter to tell when its the last call before timeout and only call prettyDOM in that situation (Im not sure about this one cause it will require some major changes and/or facing possible racing conditions here)

that's what I would've done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: browser Issues and PRs related to the browser runner p4-important Violate documented behavior or significantly improves performance (priority)
Projects
Status: Approved
Development

No branches or pull requests

2 participants