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

Database:onDisconnect() callback feature request #135

Open
blobles-dev opened this issue Jun 6, 2024 · 5 comments
Open

Database:onDisconnect() callback feature request #135

blobles-dev opened this issue Jun 6, 2024 · 5 comments

Comments

@blobles-dev
Copy link

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
Copy link
Owner

FredyH commented Jun 7, 2024

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
Copy link
Author

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
Copy link
Owner

FredyH commented Jun 7, 2024

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
Copy link
Author

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.

@FredyH
Copy link
Owner

FredyH commented Sep 15, 2024

This is now done in the latest beta release, please let me know if it works for you!

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

No branches or pull requests

2 participants