Merge pull request #128 from tjenkinson/automatic-forkable-reader-gc

Switch to `forkable-iterator` package so that GC can be automatic on `ForkableReader`

jest.config.js

@@ -5,6 +5,7 @@ module.exports = {
   testEnvironment: 'node',
   rootDir: 'src',
   restoreMocks: true,
+  setupFilesAfterEnv: ['<rootDir>/jest-setup.js'],
   globals: {
     'ts-jest': {
       tsconfig: path.resolve(__dirname, './tsconfig.test.json'),

package.json

@@ -21,11 +21,14 @@
     "lint": "eslint --ext .ts --max-warnings 0 --cache src",
     "prettier": "prettier --write .",
     "prettier:check": "prettier --check .",
-    "test": "jest",
-    "test:ci": "jest --coverage",
+    "test": "node --expose-gc ./node_modules/.bin/jest",
+    "test:ci": "node --expose-gc ./node_modules/.bin/jest --coverage",
     "prepare": "husky install",
     "lint-staged": "lint-staged"
   },
+  "engines": {
+    "node": ">=14.0.0"
+  },
   "repository": {
     "type": "git",
     "url": "git+https://github.com/tjenkinson/redos-detector.git"
@@ -51,6 +54,7 @@
     "eslint": "8.22.0",
     "eslint-config-prettier": "8.5.0",
     "eslint-plugin-redos-detector": "2.0.12",
+    "forkable-iterator": "1.1.1",
     "husky": "8.0.1",
     "jest": "28.1.3",
     "lint-staged": "13.0.3",

renovate.json

@@ -9,7 +9,10 @@
   "rangeStrategy": "bump",
   "packageRules": [
     {
-      "matchPackageNames": ["eslint-plugin-redos-detector"],
+      "matchPackageNames": [
+        "eslint-plugin-redos-detector",
+        "forkable-iterator"
+      ],
       "stabilityDays": 0
     },
     {

src/character-reader/character-reader-level-2.ts

@@ -23,6 +23,7 @@ import {
 } from 'regjsparser';
 import { Groups, LookaheadStack } from '../nodes/group';
 import { CharacterGroups } from '../character-groups';
+import { fork } from 'forkable-iterator';
 import { MyRootNode } from '../parse';
 import { NodeExtra } from '../node-extra';
 import { QuantifierStack } from '../nodes/quantifier';
@@ -89,10 +90,7 @@ function isReaderAtEnd(
     return false;
   };
 
-  const fork = reader.fork();
-  const atEnd = isAtEndUnbounded(fork);
-  fork.dispose();
-  return atEnd;
+  return isAtEndUnbounded(fork(reader));
 }
 
 /**

src/checker-reader.ts

@@ -30,6 +30,7 @@ import {
   CharacterReaderLevel2Value,
 } from './character-reader/character-reader-level-2';
 import { BackReferenceStack } from './character-reader/character-reader-level-1';
+import { fork } from 'forkable-iterator';
 import { InfiniteLoopTracker } from './infinite-loop-tracker';
 import { last } from './arrays';
 import { MyFeatures } from './parse';
@@ -156,19 +157,13 @@ export function* buildCheckerReader(input: CheckerInput): CheckerReader {
     atomicGroupsInSync,
     level,
   }: StartThreadInput): Reader<CheckerReaderValue> {
-    const dispose = (): void => {
-      leftStreamReader.dispose();
-      rightStreamReader.dispose();
-    };
-
     if (level >= stackOverflowLimit) {
       stackOverflow = true;
     }
 
     for (;;) {
       if (!timedOut) timedOut = Date.now() > latestEndTime;
       if (timedOut || stackOverflow) {
-        dispose();
         return;
       }
       const nextLeft: ReaderResult<
@@ -187,7 +182,7 @@ export function* buildCheckerReader(input: CheckerInput): CheckerReader {
           leftInitial: null,
           leftStreamReader: buildForkableReader(nextLeft.value.reader()),
           level: level + 1,
-          rightStreamReader: rightStreamReader.fork(),
+          rightStreamReader: fork(rightStreamReader),
           trail,
         });
 
@@ -210,7 +205,7 @@ export function* buildCheckerReader(input: CheckerInput): CheckerReader {
           atomicGroupsInSync,
           infiniteLoopTracker: infiniteLoopTracker.clone(),
           leftInitial: nextLeft,
-          leftStreamReader: leftStreamReader.fork(),
+          leftStreamReader: fork(leftStreamReader),
           level: level + 1,
           rightStreamReader: buildForkableReader(nextRight.value.reader()),
           trail,
@@ -226,15 +221,13 @@ export function* buildCheckerReader(input: CheckerInput): CheckerReader {
       }
 
       if (++stepCount > input.maxSteps) {
-        dispose();
         return;
       }
 
       if (
         (nextLeft.done && nextLeft.value === 'abort') ||
         (nextRight.done && nextRight.value === 'abort')
       ) {
-        dispose();
         return;
       }
 
@@ -243,7 +236,6 @@ export function* buildCheckerReader(input: CheckerInput): CheckerReader {
       }
 
       if (nextLeft.done || nextRight.done) {
-        dispose();
         return;
       }
 
@@ -268,7 +260,6 @@ export function* buildCheckerReader(input: CheckerInput): CheckerReader {
           (leftPassedStartAnchor || rightPassedStartAnchor)) ||
         leftPassedStartAnchor !== rightPassedStartAnchor
       ) {
-        dispose();
         return;
       }
 
@@ -278,7 +269,6 @@ export function* buildCheckerReader(input: CheckerInput): CheckerReader {
         // something before a lookahead can't give something up to be consumed in the lookahead
         // therefore we only want to compare instances that start a lookahead in sync
         // I.e. `a+(?=a+)` is fine but `a+(?=a+a+)` is not
-        dispose();
         return;
       }
 
@@ -299,7 +289,6 @@ export function* buildCheckerReader(input: CheckerInput): CheckerReader {
         if (leftAndRightIdentical) {
           // left and right have been identical to each other, and we are now entering an infinite
           // portion, so bail
-          dispose();
           return;
         }
       }
@@ -334,7 +323,6 @@ export function* buildCheckerReader(input: CheckerInput): CheckerReader {
         if (leftValue.node === rightValue.node) {
           yield { type: checkerReaderTypeInfiniteLoop };
         }
-        dispose();
         return;
       }
 
@@ -343,7 +331,6 @@ export function* buildCheckerReader(input: CheckerInput): CheckerReader {
         rightValue.characterGroups
       );
       if (isEmptyCharacterGroups(intersection)) {
-        dispose();
         return;
       }
 
@@ -361,7 +348,6 @@ export function* buildCheckerReader(input: CheckerInput): CheckerReader {
         // if we are not entering/leaving an atomic group in sync
         // then bail, as atomic groups can't give something up to be
         // consumed somewhere else
-        dispose();
         return;
       }
 

src/jest-setup.js

@@ -0,0 +1,9 @@
+afterAll(() => {
+  // prevents
+  // "A worker process has failed to exit gracefully and has been force exited. This is likely caused by tests leaking due to improper teardown. Try running with --detectOpenHandles to find leaks. Active timers can also cause this, ensure that .unref() was called on them."
+  // happening
+  // This seems to be because the process is taking too long to terminate, as a result of GC taking too long during the timeout period
+  // Doing this moves the GC to before the request for the process to stop where the timeout is running
+  // See https://github.com/facebook/jest/pull/13139
+  global.gc?.();
+});

src/reader.test.ts

@@ -1,48 +1,23 @@
-import {
-  buildArrayReader,
-  buildForkableReader,
-  ForkableReader,
-} from './reader';
+import { buildArrayReader, chainReaders, emptyReader } from './reader';
 
 describe('Reader', () => {
-  describe('ForkableReader', () => {
-    let reader: ForkableReader<number, void>;
-    beforeEach(() => {
-      reader = buildForkableReader(buildArrayReader([1, 2, 3]));
-    });
-
+  describe('chainReaders()', () => {
     it('yields the correct items', () => {
-      const fork = reader.fork();
-      expect(fork.next().value).toBe(1);
-      expect(fork.next().value).toBe(2);
-
+      const reader = chainReaders([
+        buildArrayReader([0, 1]),
+        buildArrayReader([2, 3]),
+      ]);
+      expect(reader.next().value).toBe(0);
       expect(reader.next().value).toBe(1);
-
-      const fork2 = reader.fork();
       expect(reader.next().value).toBe(2);
-
-      expect(fork2.next().value).toBe(2);
-
       expect(reader.next().value).toBe(3);
-      expect(reader.next().value).toBe(undefined);
-
-      expect(fork.next().value).toBe(3);
-      expect(fork.next().value).toBe(undefined);
-
-      expect(fork2.next().value).toBe(3);
-      expect(fork2.next().value).toBe(undefined);
-    });
-
-    it('throws if next() is called after dispose', () => {
-      reader.dispose();
-      expect(() => reader.next()).toThrowError(
-        'Internal error: reader disposed'
-      );
+      expect(reader.next().done).toBe(true);
     });
+  });
 
-    it('allows dispose() to be called multiple times', () => {
-      reader.dispose();
-      reader.dispose();
+  describe('emptyReader()', () => {
+    it('is immediately done', () => {
+      expect(emptyReader().next().done).toBe(true);
     });
   });
 });

src/reader.ts

@@ -1,87 +1,22 @@
+import { buildForkableIterator, ForkableIterator } from 'forkable-iterator';
+
 export type Reader<T, TReturn = void> = Iterator<T, TReturn>;
 export type ReaderResult<T, TReturn = void> = IteratorResult<T, TReturn>;
 
 // eslint-disable-next-line @typescript-eslint/no-empty-function
 export function* emptyReader<T>(): Reader<T> {}
 
-export type ForkableReader<T, TReturn = void> = Reader<T, TReturn> & {
-  dispose(): void;
-  fork(): ForkableReader<T, TReturn>;
-};
+export type ForkableReader<T, TReturn = void> = ForkableIterator<T, TReturn>;
 
 /**
  * Returns a reader that can be forked with the `fork` function.
- * To prevent memory leaks `dispose` should be called when finished.
  *
  * The source reader must not be read from directly.
  */
 export function buildForkableReader<T, TReturn = void>(
   sourceReader: Reader<T, TReturn>
 ): ForkableReader<T, TReturn> {
-  const onItem: Array<(item: T) => void> = [];
-  let sourceDone = false;
-  let returnVal: TReturn;
-
-  const readSource = (): void => {
-    if (sourceDone) return;
-    const result = sourceReader.next();
-    if (!result.done) {
-      onItem.forEach((fn) => fn(result.value));
-    } else {
-      onItem.splice(0, onItem.length);
-      sourceDone = true;
-      returnVal = result.value;
-    }
-  };
-
-  const makeFork = (initialPendingItems: T[]): ForkableReader<T, TReturn> => {
-    const pendingItems: T[] = [...initialPendingItems];
-    let handler: ((item: T) => void) | null = null;
-    if (!sourceDone) {
-      handler = (item: T): void => void pendingItems.push(item);
-      onItem.push(handler);
-    }
-
-    const readSourceIfNeeded = (): void => {
-      if (!pendingItems.length) {
-        readSource();
-      }
-    };
-
-    let disposed = false;
-    const ensureNotDisposed = (): void => {
-      if (disposed) throw new Error('Internal error: reader disposed');
-    };
-
-    const fork = (): ForkableReader<T, TReturn> => {
-      ensureNotDisposed();
-      return makeFork(pendingItems);
-    };
-
-    return {
-      // technically does not need to be called if the source has been drained
-      dispose(): void {
-        if (disposed) return;
-        disposed = true;
-        if (handler) {
-          const index = onItem.indexOf(handler);
-          onItem.splice(index, 1);
-        }
-      },
-      fork,
-      next(): ReaderResult<T, TReturn> {
-        ensureNotDisposed();
-        readSourceIfNeeded();
-        if (pendingItems.length) {
-          // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
-          return { value: pendingItems.shift()! };
-        }
-        return { done: true, value: returnVal };
-      },
-    };
-  };
-
-  return makeFork([]);
+  return buildForkableIterator(sourceReader);
 }
 
 /**