Register for your free account! | Forgot your password?

Go Back   elitepvpers > MMORPGs > Conquer Online 2 > CO2 Programming
You last visited: Today at 22:28

  • Please register to post and access all features, it's quick, easy and FREE!

Advertisement



MySql wrapper using parameters

Discussion on MySql wrapper using parameters within the CO2 Programming forum part of the Conquer Online 2 category.

Reply
 
Old   #1
 
U2_Caparzo's Avatar
 
elite*gold: 0
Join Date: Aug 2011
Posts: 314
Received Thanks: 90
MySql wrapper using parameters

This a MySql wrapper i coded to make easier to use this type of database
it's working using parameters, so there is no chance of MySql injection (or at least decreased it), but it could be a bit slower.
i'm sorry if this is a totally wrong way of do it, it could be because my inexperience at C# and MySql syntax(this is the first time that i really try learn about MySql).
Also the update methods(Update, Insert and Delete) are being executed by a thread to get the less possible delay when doing one of these types of consults(a thread and a queue, it can have more than 1 thread working, but can cause some bugs when inserting and updating the same row with no pause.

again, srry if this is a bad way of do it, i pm'ed cptsky if he could do a fast check to the code to evade any possible flaming, but looks like he is almost all the time disconnected, so hope i can get constructive criticism

Old

First revision
U2_Caparzo is offline  
Old 05/09/2013, 13:13   #2
 
_DreadNought_'s Avatar
 
elite*gold: 28
Join Date: Jun 2010
Posts: 2,226
Received Thanks: 868
Oh - The code you linked is very specific to CO.

(That will come those sneaky mods from moving it )
_DreadNought_ is offline  
Thanks
1 User
Old 05/09/2013, 13:51   #3
 
elite*gold: 0
Join Date: Oct 2009
Posts: 768
Received Thanks: 550
This looks good, but I have some suggestions and questions.

First is: Why don't you allow remote servers for the TCP protocol? For pipes it makes sense that the host would be the same machine but some people prefer the mysql server elsewhere (unlikely in CO's case though).

Second: Some times even 60 ms won't be enough so you should leave mysql handle it's own cleaning up. Considering we're not dealing with only a few connections, in time mysql will get some congestion problems and 60 ms wont be enough to execute whatever you want to do. I am reffering to the ConnectionLifetime.

Third: I dislike your reader. While it might be easier to write than that good old DataAdapter, DataSet and DataRow read, yours will keep a mysql driver open a tad more than the latter. When handling sql you want the drivers to be ready to be used by other parts of the server and thus you should dispose of the connection as soon as possible. Right now you only do that only after you finish reading. (I don't mean the ExecuteReader() function).

Last: Do not use something like a thread pool to handle your mysql queries. The mysql server itself is multi threaded so you dont need to bother doing it yourself. Also keep in mind that by doing so, if your threads get overwhelmed with queries your characters and stuff wont save and you'll allow people to create item duplicates or trade stuff relogin and have them back. Keep mysql on the same threads of those you use to handle the client's packet handlers if possible.
-impulse- is offline  
Thanks
1 User
Old 05/09/2013, 16:44   #4
 
nTL3fTy's Avatar
 
elite*gold: 0
Join Date: Jun 2005
Posts: 692
Received Thanks: 353
I believe there is a MySqlConnectionStringBuilder for the specific purpose of building connection strings...
nTL3fTy is offline  
Thanks
1 User
Old 05/09/2013, 18:21   #5
 
Spirited's Avatar
 
elite*gold: 12
Join Date: Jul 2011
Posts: 8,283
Received Thanks: 4,192
It looks like everything that could be said regarding criticism has been said, so I'm just going to praise you. Your multithreading looks fine in regards to concurrent command handling. Keep in mind that MySql is already a multithreaded service though. The only thing your server is really using that thread worker for is to handle the time it takes to execute commands. You were smart in keeping it at two threads. You really don't need to allocate more threads for something like this. It might be argued that you don't need this at all if you're using asynchronous/IO completion ports or a non-blocking socket system. Sometimes you also want the processing thread to wait on that command executing to ensure that it was added successfully. Just a few things to think about.
Spirited is offline  
Thanks
1 User
Old 05/09/2013, 20:07   #6
 
U2_Caparzo's Avatar
 
elite*gold: 0
Join Date: Aug 2011
Posts: 314
Received Thanks: 90
Quote:
Originally Posted by -impulse- View Post
First is: Why don't you allow remote servers for the TCP protocol? For pipes it makes sense that the host would be the same machine but some people prefer the mysql server elsewhere (unlikely in CO's case though).
I will check that, would be a good idea for a more portable and usable code

Second: Some times even 60 ms won't be enough so you should leave mysql handle it's own cleaning up. Considering we're not dealing with only a few connections, in time mysql will get some congestion problems and 60 ms wont be enough to execute whatever you want to do. I am reffering to the ConnectionLifetime.
I based the connection strings from this site, and it says that the ConnectionLifetime value is in seconds :s
Third: I dislike your reader. While it might be easier to write than that good old DataAdapter, DataSet and DataRow read, yours will keep a mysql driver open a tad more than the latter. When handling sql you want the drivers to be ready to be used by other parts of the server and thus you should dispose of the connection as soon as possible. Right now you only do that only after you finish reading. (I don't mean the ExecuteReader() function).
I totally understand, i thought that the Pooling would could handle this kind of situations(anyways, i didn't know about this point, it's something that should be coded better), would be the one that u coded for your 5165 source a good example?
Last: Do not use something like a thread pool to handle your mysql queries. The mysql server itself is multi threaded so you dont need to bother doing it yourself. Also keep in mind that by doing so, if your threads get overwhelmed with queries your characters and stuff wont save and you'll allow people to create item duplicates or trade stuff relogin and have them back. Keep mysql on the same threads of those you use to handle the client's packet handlers if possible.
While i was testing before start coding this i noticed that execute a query could take some ms(even more in the first execution) and again i thought that use a thread to execute them could allow the server use those ms to execute the packets handlers, this time i can't understand very much since almost everything(in fact, i think that everything) is handled in the server itself, mi lack of knowledge maybe is making me blind this time because i can't see how it would be 'exploited'
Quote:
Originally Posted by nTL3fTy View Post
I believe there is a MySqlConnectionStringBuilder for the specific purpose of building connection strings...
i surely will check that later, i'm kinda busy for the moment, thanks for the advice
Quote:
Originally Posted by Fаng View Post
It looks like everything that could be said regarding criticism has been said, so I'm just going to praise you. Your multithreading looks fine in regards to concurrent command handling. Keep in mind that MySql is already a multithreaded service though. The only thing your server is really using that thread worker for is to handle the time it takes to execute commands. You were smart in keeping it at two threads. You really don't need to allocate more threads for something like this. It might be argued that you don't need this at all if you're using asynchronous/IO completion ports or a non-blocking socket system. Sometimes you also want the processing thread to wait on that command executing to ensure that it was added successfully. Just a few things to think about.
i have for sure make some testings in real cases, i will try to emulate a server later to check that, ofc just socket, db, and a client
Thanks u all guys, i got really good advices to improve this, maybe this weekend i will have some time update it
U2_Caparzo is offline  
Old 05/10/2013, 00:12   #7


 
CptSky's Avatar
 
elite*gold: 0
Join Date: Jan 2008
Posts: 1,444
Received Thanks: 1,176
Quote:
Originally Posted by U2_Caparzo View Post
again, srry if this is a bad way of do it, i pm'ed cptsky if he could do a fast check to the code to evade any possible flaming, but looks like he is almost all the time disconnected, so hope i can get constructive criticism
Sorry. As I know nobody is really working on EoF, and as I don't check the CO2 section on the other forum (I'm active here), I never saw your PM... You have more chance when PM'ing me here.

---

Overall

You often compare the string to string.Empty, but if the string is null it won't enter in the condition, and well, for example, a null connection string won't be valid ! I wouldn't compare to an empty string, but directly call IsNullOrEmpty() on the string. It's cleaner, respect all use cases, and probably more optimized.

When using switches, I always add a default case. It may be important (like in a constructor) to be sure to have a valid object, or to signal incoherent behaviour. It's not an obligation (although some compiler do force the presence of a default case in C++).

MySQL is multi-threaded as already stated. You don't need, and shouldn't, handle it yourself. Plus, no thread-pool. You must execute the queries sequentially for a client. Imagine the case of two commands. One set the money to 200$, the second set the money to 100$ (you bought something else). In a thread-pool, you have chance two executes the second command before the first one... It opens then door to plenty of abuse.

You don't follow one naming convention through the project. Adopt one and stick with it

DBCore.cs

Your connection string doesn't use escape characters to securize (?) the SQL string. I know it should be called with valid params, but always better to protect the SQL string.

ConnectionLifeTime is way too small. It will work on small scale local server.

DBEnums

Stupid, but you seems to use two naming convention for internal members of enums. I would stick with one. It's not a real problem, but I love beautiful code

DBThread

What happens if if(commands.TryDequeue(out cmd)) fail ? Well, it's an illogical case, no ? Why the method should be called when the commands queue is empty ? It's a good check, but it shouldn't happens and should be checked probably before ?

What happens if you call Start() several times ? Old threads will continue ?

using(var conn = DBCore.GetNewConnection())
You know the type right ? Don't use var for this case, it's useless.


MySQLCommand

Not convinced by the return this;... After all, you already have the object. I would probably return a boolean instead.

What happens if a parameter is null ? What happens if a parameter is invalid and inject some code in the command ? (SQL injection) Use escape characters and validate the input.

MySQLReader
Same thing than the MySQLCommand almost.

For all the ReadX() why you return the instance instead of returning the value ? It's pretty useless to use an out parameter. Also, you don't use templates, right ? So, the default keyword is useless and you should directly attribute the default value. It's not a generic code, I don't see the point of using default which isn't explicit at all.

ThreadFactory

No way to stop the threads ? Else it's fine.


I think the others have done a fine review.
CptSky is offline  
Thanks
3 Users
Old 05/10/2013, 04:21   #8
 
U2_Caparzo's Avatar
 
elite*gold: 0
Join Date: Aug 2011
Posts: 314
Received Thanks: 90
Quote:
Originally Posted by CptSky View Post
Sorry. As I know nobody is really working on EoF, and as I don't check the CO2 section on the other forum (I'm active here), I never saw your PM... You have more chance when PM'ing me here.

---

Overall

You often compare the string to string.Empty, but if the string is null it won't enter in the condition, and well, for example, a null connection string won't be valid ! I wouldn't compare to an empty string, but directly call IsNullOrEmpty() on the string. It's cleaner, respect all use cases, and probably more optimized.

Damn, i always forgot those comparissons :s, i'm editing it rigth now so it should have no possible errors(ofc its possible, but should not be a coding error)

When using switches, I always add a default case. It may be important (like in a constructor) to be sure to have a valid object, or to signal incoherent behaviour. It's not an obligation (although some compiler do force the presence of a default case in C++).

I agreed totally, but its like a coder using this wrapper were going to crash it intentionally parsing an invalid input to the enum, i'll gonna work on that though.

MySQL is multi-threaded as already stated. You don't need, and shouldn't, handle it yourself. Plus, no thread-pool. You must execute the queries sequentially for a client. Imagine the case of two commands. One set the money to 200$, the second set the money to 100$ (you bought something else). In a thread-pool, you have chance two executes the second command before the first one... It opens then door to plenty of abuse.

i kinda noticed that possible error, atm i was using 1 single thread to execute the commands, the main reason of using this thread is because i want to use the time required for create/open a connection/execute the query to generate a faster answer(packet send) to the client, something like this 'schema'


You don't follow one naming convention through the project. Adopt one and stick with it

Well, this is kinda difficult to me :/, i will have to read some guides of global naming convention(i'm sure there are), i will do my best to make it look better since it will help during any other project.

DBCore.cs

Your connection string doesn't use escape characters to securize (?) the SQL string. I know it should be called with valid params, but always better to protect the SQL string.
ConnectionLifeTime is way too small. It will work on small scale local server.

I just edited that class, it's now using the MySqlConnectionStringBuilder as
recommended by nTL3fTy (and any other coder who knows about this i guess), and checking the strings so now it HAS to be a valid connection string except if someone really want to have an invalid one (wrong db names, wrong user, etc...)
about the ConnectionLifeTime, i'm sure that i have read that it's expressed in seconds, and i thought that 1 minute would be enough


DBEnums

Stupid, but you seems to use two naming convention for internal members of enums. I would stick with one. It's not a real problem, but I love beautiful code

just noticed that obvious and idiot style error , just changed it.

DBThread

What happens if if(commands.TryDequeue(out cmd)) fail ? Well, it's an illogical case, no ? Why the method should be called when the commands queue is empty ? It's a good check, but it shouldn't happens and should be checked probably before ?

Well, this time i got a little confused

What happens if you call Start() several times ? Old threads will continue ?

Going to add a check to that

using(var conn = DBCore.GetNewConnection())
You know the type right ? Don't use var for this case, it's useless.


MySQLCommand

Not convinced by the return this;... After all, you already have the object. I would probably return a boolean instead.

it was just to use Command.Insert(..).Insert(...).Execute();
instead of
Command.Insert(...);
Command.Insert(...);
Command.Execute();


What happens if a parameter is null ? What happens if a parameter is invalid and inject some code in the command ? (SQL injection) Use escape characters and validate the input.

would not Parameters.AddWithValue() do the job?... all my life is a lie , please confirm that so i will start doing some sanity checks, i'm searching about it though.

MySQLReader
Same thing than the MySQLCommand almost.

For all the ReadX() why you return the instance instead of returning the value ? It's pretty useless to use an out parameter. Also, you don't use templates, right ? So, the default keyword is useless and you should directly attribute the default value. It's not a generic code, I don't see the point of using default which isn't explicit at all.

I will follow -impulse-'s advice, i'm going to research about it, and then i expect to do something a bit better, i will follow your advice too obviously, will edit a bit the code, as summary, change a bit of less typing for a more functional code

ThreadFactory

No way to stop the threads ? Else it's fine.
could not figure ways to do so except using Abort(), i will look for others methods, i kinda dislike using Thread.Abort().

I think the others have done a fine review.
Thanks you , i wrote some answers in bold font.
U2_Caparzo is offline  
Old 05/10/2013, 11:39   #9
 
InfamousNoone's Avatar
 
elite*gold: 20
Join Date: Jan 2008
Posts: 2,012
Received Thanks: 2,885
Quote:
using(var conn = DBCore.GetNewConnection())
You know the type right ? Don't use var for this case, it's useless.
I don't see the problem with this.
InfamousNoone is offline  
Old 05/11/2013, 13:12   #10


 
Korvacs's Avatar
 
elite*gold: 20
Join Date: Mar 2006
Posts: 6,126
Received Thanks: 2,518
Quote:
Originally Posted by InfamousNoone View Post
I don't see the problem with this.
'var' should only be used when working with anonymous types, beyond that your just making your code easier to read while sacrificing clarity. For example using 'var' in that case is easier than the full object name yes, but it isn't clear what 'DBCore.GetNewConnection()' returns except that its some form of new connection.

So in this case the type should be declared explicitly.
Korvacs is offline  
Thanks
1 User
Old 05/11/2013, 14:38   #11
 
elite*gold: 0
Join Date: Mar 2013
Posts: 118
Received Thanks: 95
Who cares, the variable is only used inside the scope of the using statement anyways, no big deal imo
Smaehtin is offline  
Old 05/11/2013, 15:11   #12


 
Korvacs's Avatar
 
elite*gold: 20
Join Date: Mar 2006
Posts: 6,126
Received Thanks: 2,518
Quote:
Originally Posted by Smaehtin View Post
Who cares, the variable is only used inside the scope of the using statement anyways, no big deal imo
Its statements like this that help contribute to a large amount of bad practice found in programming all around the world.
Korvacs is offline  
Old 05/11/2013, 16:16   #13
 
elite*gold: 0
Join Date: Mar 2013
Posts: 118
Received Thanks: 95
Quote:
Originally Posted by Korvacs View Post
Its statements like this that help contribute to a large amount of bad practice found in programming all around the world.
Haha, have you ever used ReSharper?
Smaehtin is offline  
Old 05/11/2013, 18:05   #14
 
_DreadNought_'s Avatar
 
elite*gold: 28
Join Date: Jun 2010
Posts: 2,226
Received Thanks: 868
Quote:
Originally Posted by Smaehtin View Post
Haha, have you ever used ReSharper?
Yeap, reSharper would most definitely agree with the OP.

I would too.

@korv


It appears this debate is one programmers have often

Also your signature is a load of ****, the host you're doing is a speed test on is local.
_DreadNought_ is offline  
Old 05/11/2013, 18:55   #15
 
elite*gold: 0
Join Date: Mar 2013
Posts: 118
Received Thanks: 95
Quote:
Originally Posted by _DreadNought_ View Post
Also your signature is a load of ****, the host you're doing is a speed test on is local.
How do I force speedtest.net to localhost??
Smaehtin is offline  
Reply

Tags
c#, mysql, wrapper


Similar Threads Similar Threads
[Release] Fixed MySql Wrapper
09/22/2013 - CO2 PServer Guides & Releases - 11 Replies
Hey everyone. This was a request from a close friend of mine who's having issues with his mysql wrapper from Impulse's source. MySql tables, columns, and values can have spaces... so here's a wrapper that fixes any issues that you might have with spaces (note: the structure was coded by Impulse, all I did was fix it): public class MySqlCommand { private MySqlCommandType _type;
MySQL wrapper used in Albetros/Impulse
03/31/2013 - CO2 Private Server - 3 Replies
Hey there, this is a question for Pro4never / Impulse. I'd like to use the MySQL wrapper used in Albetros/Impulse and implement it in my source (with you guys credits ofcourse). It's easy to use when pulling data from a database compared to how I have it setup now (1 huge SQL line lol). I referenced the MySqlHandler.dll, Could you explain how to open a connection that will pair with the dll? I tried the same method used to connect with MySql.Data.dll but it doesn't seem to work. Much...
G1's 9D startup parameters
11/18/2010 - 9Dragons - 5 Replies
This one may be useless but if you want to start G1's 9D w/o a launcher try these codes - pass then as parameters: -C2EZWTWTCTRCC5T3956JWA43XU -Q2NWVUHQBJR3F6N390C0Q5N3XU -A2CSOEI3RVRCENT393BCCSG3XU -U2BZAV1QBCRQX0Q39NO5SQ23XU -Q2DOUQ8S6GR0IQ0Q90NSRU63XU -A2NRSTXC3HRQ6UA39SC0ZZY3XU -E2NRTO2CNQRSRDJQ9T3JSNG3XU -U2RCD50TQVRNINCQ9QO0ASI3XU
Parameters?
10/15/2009 - Cabal Online - 2 Replies
This is the reply from Nova I got when I asked what some things did... and quite honestly I didn't fully understand his reply, to my knowledge basically only parameter 4 is the only one that is needed to be changed but my issue is that every time I craft a get a different "EBP+0x0546: Return Address of Calling Function" the only real constant that I notice between the different SocketTrace tests is the "flag" which according to Nova would be EBP+0x27 <<<<<<<<<<<<&l t; yes that's the REAL numbers I...
what are the Parameters of being banned
07/26/2009 - Grand Chase Philippines - 25 Replies
just want to know up to what extent of hacking will you be banned what are the process and is there a safe way to hack? (not to mention public hacking?) thanks



All times are GMT +1. The time now is 22:28.


Powered by vBulletin®
Copyright ©2000 - 2026, Jelsoft Enterprises Ltd.
SEO by vBSEO ©2011, Crawlability, Inc.
This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.

Support | Contact Us | FAQ | Advertising | Privacy Policy | Terms of Service | Abuse
Copyright ©2026 elitepvpers All Rights Reserved.