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

fix(worker): return this.closing when calling close #153

Merged
merged 6 commits into from
Apr 10, 2020

Conversation

manast
Copy link
Contributor

@manast manast commented Mar 3, 2020

No description provided.

Copy link
Contributor

@onehorsetown onehorsetown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I was just about to submit a PR for this w/ one additional change.

Look at where the promise created in Worker.close calls super.close()

await super.close();

QueueBase.close overwrites this.closing, thereby introducing a subtle bug.

Actually QueueBase.close itself has a bug where, if it is called twice, you end up overwriting the existing promise and end up closing the connection twice.

Perhaps QueueBase.close is better implemented as:

  close() {
    if (!this.closing) {
      this.closing = this.connection.close();
    }

    return this.closing;
  }

Which both prevents the connection from being closed twice and keeps the call to super.close in Worker.close from overwriting this.closing.

@manast manast merged commit d7888ae into master Apr 10, 2020
@manast manast deleted the fix/return-closing-promise-in-worker branch April 10, 2020 15:20
@manast
Copy link
Contributor Author

manast commented Apr 10, 2020

🎉 This PR is included in version 1.8.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

@manast
Copy link
Contributor Author

manast commented Jul 3, 2020

🎉 This PR is included in version 1.8.14 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

2 participants