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 issue where subscription server data channel closing gives error #354

Merged
merged 1 commit into from
Sep 30, 2024

Conversation

HaraldNordgren
Copy link
Contributor

@HaraldNordgren HaraldNordgren commented Sep 27, 2024

Handles errors discussed here: #347 (comment)

I have:

  • Written a clear PR title and description (above)
  • Signed the Khan Academy CLA
  • Added tests covering my changes, if applicable
  • Included a link to the issue fixed, if applicable
  • Included documentation, for new features
  • Added an entry to the changelog

@HaraldNordgren HaraldNordgren mentioned this pull request Sep 27, 2024
6 tasks
@HaraldNordgren HaraldNordgren marked this pull request as ready for review September 27, 2024 15:52
@HaraldNordgren HaraldNordgren changed the title Handle websocket data channel closing Handle subscription data channel closing Sep 27, 2024
@HaraldNordgren HaraldNordgren force-pushed the handle_channel_closing branch 2 times, most recently from 6246e7e to 141dd9e Compare September 27, 2024 18:38
@HaraldNordgren HaraldNordgren changed the title Handle subscription data channel closing Handle subscription server data channel closing without giving error Sep 27, 2024
@HaraldNordgren HaraldNordgren changed the title Handle subscription server data channel closing without giving error Fix issue where subscription server data channel closing gives error Sep 27, 2024
Copy link
Collaborator

@benjaminjkraft benjaminjkraft left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me. It's kinda funny we end up with reflect for this, but it seems fine, and better than adding a bunch of new plumbing to generate a single close call.

I'll wait a couple days in case @matthieu4294967296moineau has anything to add.

@HaraldNordgren
Copy link
Contributor Author

Yeah, I don't love the reflect part, although it does solves the problem. The reason I still went with it was because I found the same thing in subscriptionsMap.Unsubscribe().

If we really don't want to reflect then a type cast could likely also solve the problem 🤗

@benjaminjkraft
Copy link
Collaborator

Ah, I guess it's not the first then!

A cast would be better, but won't work because we don't know the concrete type, right?

@HaraldNordgren
Copy link
Contributor Author

Ah, right!

@matthieu4294967296moineau
Copy link
Contributor

Ah, I guess it's not the first then!

A cast would be better, but won't work because we don't know the concrete type, right?

yes this is why I used it in a first place to close the channel when unsubscribing

The fix looks good to me, I could test it and it worked fine 👍

@HaraldNordgren
Copy link
Contributor Author

@benjaminjkraft @matthieu4294967296moineau Great! Thanks for the feedback!

Let's merge? 🚀

@benjaminjkraft benjaminjkraft merged commit e030ff1 into Khan:main Sep 30, 2024
4 checks passed
@HaraldNordgren HaraldNordgren deleted the handle_channel_closing branch October 2, 2024 08:59
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.

3 participants