FredyH / MySQLOO

MySQLOO
GNU Lesser General Public License v2.1
137 stars 55 forks source link

Database:onDisconnect() callback feature request #135

Open blobles-dev opened 3 months ago

blobles-dev commented 3 months ago

My current setup is each player has their own connection, this ensures that any actions they do with my inventory system are completed in order - however, my global table of connections can become stale and essentially a memory leak.

'example'
player join:
         create sql connection in globals SQL_CONS[ply:SteamID64()]
player disconnect:
          SQL_CONS[ply:SteamID64()]:disconnect(true) -- wait for the queue to clear

With the above scenario, SQL_CONS will still maintain an index with an disconnected connection that is not needed.

I have no way of detecting the successful disconnection to clear out the value in the global table - I'm going to write a timer that will check the connections every 10 minutes or so and drop stale references, but it would be nice to clear it out as soon as the queue is empty.

FredyH commented 3 months ago

So if I understand correctly you would require a Database.onDisconnected( db ) callback, that is called after all the queries are finished. I think this callback makes sense so I will release an update with it when I next get around to it.

In the meantime, you can already implement this yourself (although somewhat more tedious) by keeping track of the number of running queries, increasing when launched, and decreasing when any callback is called (abort/error/success). Then, after the counter hits 0, you can be sure that everything is finished and you can clear it up.

Apart from that, I am not sure why you don't just immediately remove the database instance from the table. If the same player reconnects you cannot reuse the old, disconnected db instance anyways, right?

Also, have you considered using a single db instance for all the queries related to inventory operations? That way you get the sequential order you want and you won't overload your database with too many connections. (most databases cannot handle 100 connections at the same time and will error.

blobles-dev commented 3 months ago

Thats correct regarding the callback. I decided that way so a player couldnt throttle the queue maliciously (only his own) somehow.

A lot of my queries rely on others/daisy-chained in the success callback For example, they use an item, the delete returns success with rows > 0 and then on the callback to update the item it was used on) i could use transactions for stuff like this but the delete would still be "success" even if 0 records If i nil the reference, then the second callback would fail

FredyH commented 3 months ago

Just for your particular use case I'd suggest ensuring that the queue isn't overloaded by rate limiting player actions. If one connection is still not enough, I'd suggest using a static pool of say 5 connections and assigning each player to one of the 5 connections when they join, that way you still have the guaranteed order of operations.

blobles-dev commented 3 months ago

Its a bit more complicated then that, I appreciate your input though. Id have to consider a lot of edge cases with different methods, id rather keep things simple given how economy driven my server is its important that weird things dont occur. Its unlikely someone will throttle their own queue, but its vital theyre in order - my own current concern is just killing the stale references in the global table. That being said its not a massive issue as the map changes every 30 minutes or so, i like to keep things clean.