fix(path): improve regex in isGlob

[ericcornelissen]

Improve the regular expression used in the isGlob function so that it does not exhibit superlinear runtime. The overhead (without this change) can be tested by adding the following test cases

assert(!isGlob("[".repeat(1_000_000) + "x"));
assert(!isGlob("{".repeat(1_000_000) + "x"));
assert(!isGlob("(".repeat(1_000_000) + "x"));

to is_glob_test.ts. I would add these to the test suite but I'm not sure how these kinds of performance-like tests should be written.

[CLAassistant]

All committers have signed the CLA.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.19%. Comparing base (fc59ca6) to head (7afa33d).
⚠️ Report is 17 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6764      +/-   ##
==========================================
+ Coverage   94.14%   94.19%   +0.05%     
==========================================
  Files         586      590       +4     
  Lines       42465    42577     +112     
  Branches     6701     6718      +17     
==========================================
+ Hits        39980    40107     +127     
+ Misses       2435     2421      -14     
+ Partials       50       49       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[kt3k]

Thanks for the PR. Looks good findings to me 👍

I would add these to the test suite but I'm not sure how these kinds of performance-like tests should be written.

Can you simply add the above assertions to is_glob_test.ts? The existing code would times out with those patterns.

[ericcornelissen]

Can you simply add the above assertions to is_glob_test.ts? The existing code would times out with those patterns.

I could. However, since the regexes are blocking, the test will just take a very long time (denoland/deno#11133 (comment) would be useful) and, from my testing, they never really time out (still passing after 35s)...

I'm not too familiar with testing in Deno, so if there's a way do let me know and I'd be happy to update (or feel free to commit the test yourself).

[kt3k]

Then how about using Worker (with data: url)? You can execute blocking operation in another thread (and can let it fail in the middle if it takes too much time):

import { isGlob } from "@std/path";
import { assert } from "@std/assert";

const worker = new Worker(`data:text/javascript,
  import { isGlob } from '@std/path';
  import { assert } from '@std/assert';
  assert(!isGlob("[".repeat(1_000_000) + "x"));
  assert(!isGlob("{".repeat(1_000_000) + "x"));
  assert(!isGlob("(".repeat(1_000_000) + "x"));
  postMessage('ok')
`, { type: "module" })
worker.addEventListener("message", (e) => {
  console.log(e.data)
  worker.terminate();
});

[ericcornelissen]

Adding the test case listed below has the desired effect, except that:

1. It requires running deno test --allow-read.

2. It outputs

   deno test path/is_glob_test.ts  --allow-read
   Check file:///deno-std/path/is_glob_test.ts
   running 1 test from ./path/is_glob_test.ts
   isGlob() ...Check data:text/javascript,          import { isGlob } from "@std/path";          isGlob("[".repeat(1_000_000) + "x");          isGlob("{".repeat(1_000_000) + "x");          isGlob("(".repeat(1_000_000) + "x");          postMessage(true);
    ok (434ms)
   
   ok | 1 passed | 0 failed (443ms)
   

// Performance
assert(
  await new Promise((resolve, reject) => {
    const worker = new Worker(`
      data:text/javascript,
      import { isGlob } from "@std/path";
      isGlob("[".repeat(1_000_000) + "x");
      isGlob("{".repeat(1_000_000) + "x");
      isGlob("(".repeat(1_000_000) + "x");
      postMessage(true);`,
      { type: "module" },
    );

    const timeout = setTimeout(() => {
      worker.terminate();
      reject("Test timed out");
    }, 1000);

    worker.addEventListener("message", (e) => {
      clearTimeout(timeout);
      resolve(e.data);
      worker.terminate();
    });

    worker.addEventListener("error", (e) => {
      clearTimeout(timeout);
      reject(e.error);
      worker.terminate();
    });
  }),
);

[ericcornelissen]

Ping @kt3k

[kt3k]

Hey, sorry for the delay, and thanks for the test case above. We added that as the test case.

Thanks for your contribution! LGTM

[ericcornelissen]

No problem! Thanks for finalizing the changes and merging 🙂