Skip to content

Potential memory leak by only adding handlers #23

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

Closed
ukstv opened this issue Mar 6, 2023 · 2 comments
Closed

Potential memory leak by only adding handlers #23

ukstv opened this issue Mar 6, 2023 · 2 comments

Comments

@ukstv
Copy link
Contributor

ukstv commented Mar 6, 2023

Consider code in the README file:

const anySignal = require('any-signal')

const userController = new AbortController()
const timeoutController = new AbortController()

const combinedSignal = anySignal([userController.signal, timeoutController.signal])
combinedSignal.addEventListener('abort', () => console.log('Abort!'))

// Abort after 1 second
const timeoutId = setTimeout(() => timeoutController.abort(), 1000)

// The user or the timeout can now abort the action
await performSomeAction({ signal: combinedSignal })
clearTimeout(timeoutId)

After all the lines are executed, there are still event handlers attached to userController.signal and timeoutController.signal. There is no code that clears them. It all works fine if userController and timeoutController both are garbage collected after clearTimeout.

Let's consider the following code, where an AbortController is long-lived, along with its AbortSignal.

class Loader {
  shutdownController = new AbortController()

  async load(id: string, options: { signal: AbortSignal }) {
    const effectiveSignal = anySignal([this.shutdownController.signal, options.signal])
    await doSomething(effectiveSignal)
    return "Success!"
  }
}

On every call to load method, you'd add an event handler to this.shutdownController.signal. It never gets removed, which leads to some minor but very constant memory leak, as well as some annoying warnings, like libp2p/js-libp2p#900

Are you open to PRs fixing this?

@jacobheun
Copy link
Owner

Are you open to PRs fixing this?

Yes, feel free to open a PR. If you can include a test that validates the event handler cleanup that would be great

@jacobheun
Copy link
Owner

Fixed in #24. This has been released in [email protected]

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

2 participants