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

the implementation of drained() api potentially can cause maximum stack exceeded error #86

Open
normanzb opened this issue Oct 1, 2024 · 0 comments · May be fixed by #87
Open

the implementation of drained() api potentially can cause maximum stack exceeded error #86

normanzb opened this issue Oct 1, 2024 · 0 comments · May be fixed by #87

Comments

@normanzb
Copy link

normanzb commented Oct 1, 2024

the drained() function wraps existing drain() callback and replace it with the new one, and it only got reset back to noop when killed, imagine if somebody call the await drained() a few thousands times in a for-loop (like a long running process), it will create a huge stack and blow when somebody calls drain() later. that will stop the promise from being resolved or even worse when somebody calls killAndDrain() it will crash.

should use an array instead or use a 3rd party event lib or just cache the drained promise, or at least reset it everytime drained (only mitigate the issue).

  function drained () {
    if (queue.idle()) {
      return new Promise(function (resolve) {
        resolve()
      })
    }

    var previousDrain = queue.drain

    var p = new Promise(function (resolve) {
      queue.drain = function () {
        previousDrain()
        resolve()
      }
    })

    return p
  }
}
@normanzb normanzb linked a pull request Oct 2, 2024 that will close this issue
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

Successfully merging a pull request may close this issue.

1 participant