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

Remove redis events on connection end #880

Merged
merged 2 commits into from Nov 9, 2022

Conversation

joksnet
Copy link
Contributor

@joksnet joksnet commented Nov 8, 2022

Fix #803. Makes the connection event listeners on and off simetrical.

@evantahler
Copy link
Member

evantahler commented Nov 8, 2022

Thank you for the contribution, @joksnet! This looks great.

The new test is failing in CI, likely because we need to wait a bit after await connection.connect(); - try adding a sleep or awaiting a redis command's completion to ensure we are connected and the events wired up

@evantahler evantahler self-requested a review November 8, 2022 21:17
@joksnet
Copy link
Contributor Author

joksnet commented Nov 8, 2022

Oh, I mixed this.listeners which is a connection's EventEmitter property and this.eventListeners that are the ones attached to the redis instance. I think with this change should be working correctly, as only error and end events are on the redis instance on connection and off on disconnections. Also changed the interface to match typescript definitions.

Which in the end seemed to be the problem was that error being attached with connection.on('error') was present in this.listeners, the loop in the disconnect was removing it but was not removing the end event.

@evantahler evantahler enabled auto-merge (squash) November 9, 2022 00:32
@evantahler evantahler merged commit 10e11e6 into actionhero:main Nov 9, 2022
@evantahler
Copy link
Member

Released! https://github.com/actionhero/node-resque/releases/tag/v9.2.1

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 this pull request may close these issues.

Worker did not remove the event listener when it ended.
3 participants