Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Clean up namespaces after client is rejected in middleware #4773

Merged
merged 3 commits into from
Jul 21, 2023

Conversation

carera
Copy link
Contributor

@carera carera commented Jul 19, 2023

The kind of change this PR does introduce

  • a bug fix
  • a new feature
  • an update to the documentation
  • a code change that improves performance
  • other

Change is fixing bug described here: #4772

@@ -758,9 +758,6 @@ export class Socket<
}

this._cleanup();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

_cleanup is called here and in two more places: https://github.com/socketio/socket.io/blob/main/lib/namespace.ts#L327-L333

I believe the this.nsp._remove(this); should be called in all these cases

lib/socket.ts Outdated
@@ -772,6 +769,9 @@ export class Socket<
*/
_cleanup() {
this.leaveAll();
this.nsp._remove(this);
this.client._remove(this);
this.connected = false;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this.client._remove(this); and this.connected = false; are needed here, since the socket is not actually connected if it's rejected by the middleware. What do you think?

Copy link
Contributor Author

@carera carera Jul 20, 2023

Choose a reason for hiding this comment

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

I agree, I just thought it does no harm since client._remove has a guard check and no-ops if the socket is not actually connected, but yeah, let me move it out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@darrachequesne darrachequesne merged commit 0731c0d into socketio:main Jul 21, 2023
@darrachequesne
Copy link
Member

Thanks a lot 👍

@carera
Copy link
Contributor Author

carera commented Jul 21, 2023

Thank you for merging. What's the release process of this repo? Do I need to do something for this to roll out?

@darrachequesne
Copy link
Member

@carera we have a few other bug fixes in progress, I'll keep you updated.

@darrachequesne
Copy link
Member

Update: released in version 4.7.2 🚀

@darrachequesne darrachequesne added this to the 4.7.2 milestone Aug 3, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants