PsychoTea / Oxide.Ext.Discord

Discord extension and integration
https://umod.org/extensions/discord
12 stars 25 forks source link

Add Resuming & Auto Reconnecting. #20

Closed Slut closed 6 years ago

Slut commented 6 years ago

Discord recommends that when a connection is dropped that you should resume it rather than creating a new identity every time. You can send 1000 identities every 24 hours.

I also realized that my "auto reconnect" method had no timer on it and thus spamming the websocket connect method and therefore causing API bans.

I have now corrected a few mistakes..

If there's anything you want me to explain/change feel free to let me know.

Slut commented 6 years ago

Here's an example of this by disconnecting the wifi until the socket closes and then enabling it again. 6okivo

PsychoTea commented 6 years ago

These changes ought to be in separate PR's. I will manually merge as necessary.

I would also like to reiterate that automatic reconnections is very dangerous feature. I can already see looking at logs the connection is being reconnected at a rate of roughly every 60 seconds. The root issue here needs to be found and resolved rather than using hazardous band-aid techniques.

Slut commented 6 years ago

Every Discord library that I've used in the past reconnects the Socket regardless. As by the Discord Developer Documentation they even say "The internet is a scary place. Disconnections happen, especially with persistent connections. Due to Discord's architecture, this is a semi-regular event and should be expected and handled."

That quote is from the Resuming section and you never handled that?

There are a few problems that I found in the extension and have fixed.

PsychoTea commented 6 years ago

That quote is from the Resuming section

Indeed it is, and nowhere in the documentation does it advise that you should spam reconnection requests when you're consistently receiving websocket errors.

and you never handled that?

Not to mention, this is a very poor implementation of resuming a connection. The issue lies deeper within how the socket is handed, and will require some heavy work.

The reason why it was never implemented before is because that code was finished before I took over from the previous writer of the library - I never needed to touch it and therefore never did. I've come to the realization now that this isn't set up right, and therefore will be rewriting a lot of the core, but this will take time. This is a much more efficient method than applying stacks of bandaid fixes.

Slut commented 6 years ago

Indeed it is, and nowhere in the documentation does it advise that you should spam reconnection requests when you're consistently receiving websocket errors.

That is correct but it's common sense to reconnect after you've been disconnected.

Not to mention, this is a very poor implementation of resuming a connection. The issue lies deeper within how the socket is handed, and will require some heavy work.

Yes but the API states that sending repetitive identities will give you issues. Therefore resuming the previous connection is a lot better than spamming identities.

I took over from the previous writer of the library

Didn't realize that sorry.

I have made 2 new PR's - the first one I added Resuming again in a much cleaner way. I also fixed a mistake where lastheartbeat was never assigned anything but 0. This may fix some issues with the socket.

Second PR is just Enums as Channel / Message types.

If there's anything that you need changing in those PR's then DM me on Discord as it'll be easier to explain.