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

TimeoutStrategy leaks listeners #81

Open
biggyspender opened this issue Nov 16, 2023 · 1 comment
Open

TimeoutStrategy leaks listeners #81

biggyspender opened this issue Nov 16, 2023 · 1 comment

Comments

@biggyspender
Copy link

biggyspender commented Nov 16, 2023

Repro in nodejs (with TS support via tsx):

import {
  ExponentialBackoff,
  TimeoutStrategy,
  handleAll,
  retry,
  timeout,
  wrap,
} from 'cockatiel';

const fail = (times: number) => {
  let c = times;
  return async () => {
    if (c-- > 0) {
      console.log('failing');
      throw Error('fail');
    }
    console.log('success');
  };
};

const timeoutPolicy = timeout(100, TimeoutStrategy.Cooperative);
const retryPolicy = retry(handleAll, {
  backoff: new ExponentialBackoff({
    initialDelay: 100,
    exponent: 2,
    maxDelay: 500,
  }),
});
const policy = wrap(retryPolicy, timeoutPolicy);

const ac = new AbortController();

const func = fail(15);

policy.execute((ctx) => func(), ac.signal);

After about 11 retries, in the output, we see:

MaxListenersExceededWarning: Possible EventTarget memory leak detected. 11 abort listeners added to [AbortSignal]. Use events.setMaxListeners() to increase limit

A cursory read of the code suggests that listeners added via deriveAbortController are not properly cleaned up in the case that the signal is not aborted (and there seems to be no mechanism by which these hanging listeners might be removed).

Here is a reproduction, running in StackBlitz (apparently only working in chromium-based browser for me 🤨 ).

@biggyspender
Copy link
Author

biggyspender commented Nov 17, 2023

I also note that toPromise offers no mechanism have its listeners removed in the case that the signal doesn't abort. This isn't covered by the PR above, but would be caught by the following test:

    const timeoutPolicy = timeout(10, TimeoutStrategy.Cooperative);

    const ac = new AbortController();

    let listenerCount = 0;

    const sig = new Proxy(ac.signal, {
      get: (signal, key, receiver) => {
        const val = Reflect.get(signal, key, receiver);
        if (key === 'addEventListener') {
          return (...args: any[]) => {
            listenerCount++;
            return Reflect.apply(val, signal, args);
          };
        }
        if (key === 'removeEventListener') {
          return (...args: any[]) => {
            listenerCount--;
            return Reflect.apply(val, signal, args);
          };
        }
        return val;
      },
    });

    await timeoutPolicy.execute(() => Promise.resolve(), sig);

    expect(listenerCount).to.eq(0);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant