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

Rewrite: Websocket #68

Closed
scragly opened this issue Aug 15, 2020 · 5 comments
Closed

Rewrite: Websocket #68

scragly opened this issue Aug 15, 2020 · 5 comments
Labels
discussion Talking about a subject, not for actioning

Comments

@scragly
Copy link
Member

scragly commented Aug 15, 2020

This issue is for discussing and collating the issues and wanted improvements of Wavelink's Websocket for the rewrite project.

If any particular point gets a lot of attention, consider creating a dedicated issue ticket to discuss it in further detail and to collate all the details specific to it there to ensure this issue is kept relatively clean and focused on the feature as a whole.

Current issues

  • re-connection loop issue?

Improvements

  • add graceful closing
  • more descriptive errors
  • look into not waiting until d.py client is ready
@scragly scragly added rewrite discussion Talking about a subject, not for actioning labels Aug 15, 2020
@WizzyGeek
Copy link
Contributor

Reconnection loop could be fixed by a lock in connect method or by awaiting it.

Graceful closing. Wait until bot is ready and then over ride the loop's signal handlers to destroy every node which closes the respective websocket with 1000, and when everything is done, we stop the loop with loop.stop

@WizzyGeek
Copy link
Contributor

WizzyGeek commented Aug 15, 2020

A very basic implementation (of closing)

90e296b

@Gelbpunkt
Copy link
Contributor

I'd very much like the custom JSON encoders as in #65 to be usable in Wavelink, but I really feel like the implementation I proposed is not ideal because it sets way too many attributes.

I would love to see it refined in the rewrite and seems like Scrag agrees with me.

I've also requested binary websocket traffic in Lavalink, but seeing the comment it won't be added in the near future.

@scragly
Copy link
Member Author

scragly commented Aug 16, 2020

I don't see a major issue with adding support to custom serialisers. If @EvieePy doesn't have any issues with ensuring it's added as a feature, we'll pin it as an feature to add.

Edit: A proper ticket has now been created for custom JSON encoders at #75 .

@WizzyGeek
Copy link
Contributor

Maybe add a close() method which complements _connect(), so that node doesn't have to get into websocket's internals to close the web socket.

@EvieePy EvieePy closed this as completed Jun 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Talking about a subject, not for actioning
Projects
None yet
Development

No branches or pull requests

4 participants