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

websocket clean up resolvers after client abruptly disconnects #1583

Conversation

RobertoOrtis
Copy link

Describe your PR and link to any relevant issues.
Extends #1248 #1248 (comment)

Websocket resolvers were not being cleaned up after the client abruptly disconnects. For example, when the users close or refresh their browsers. This fixes it by cleaning active resolvers whenever a sendConnectionError is called.

I have:

  • Added tests covering the bug / feature (see testing)
  • Updated any relevant documentation (see docs)


c.write(&operationMessage{Type: connectionErrorMsg, Payload: b})
c.write(&operationMessage{Type: connectionErrorMsg, Payload: b})
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm the above change makes this a no-op, right? Is there something missing from this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to write same error in loop here?

c.mu.Lock()
c.conn.WriteJSON(msg)
c.mu.Unlock()
if msg.Type == "data" {
Copy link
Contributor

Choose a reason for hiding this comment

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

this would also ignore keepAlive, connected, and ack messages. I don't think this is what we want

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah @jaredly is right. We cannot afford to miss other types of messages here.

@StevenACoffman
Copy link
Collaborator

@RobertoOrtis can you address Jared's review above? Thanks!

@StevenACoffman
Copy link
Collaborator

StevenACoffman commented Nov 22, 2021

@zdraganov This is another PR that seems to maybe overlap a little with some of the other more recently merged websock PRs. Are you interested in freshening this up and adding a new PR for the remaining valuable bits?

@zdraganov
Copy link
Contributor

I will take a look.

@zdraganov
Copy link
Contributor

I think this is more related to this one #1728. I think this can be closed as it's a duplicate. @RobertoOrtis do you think there's something more on top of the mentioned PR?

@StevenACoffman
Copy link
Collaborator

I appreciate your contribution, but I am closing this as a duplicate. If there's something else I missed, please don't hesitate to open a new PR.

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

Successfully merging this pull request may close these issues.

6 participants