Server player limit bug

#1 If you give me a fix, I'll put it in the engine. :)

#2 I've tried to reproduce this on several different builds with no success. And looking at the code in the latest head, I don't even see how this could be possible.

I'd be willing to bet a small wager that Eric has an error or glitch in his own scripts.


Eric, did you do this test on a basic HEAD or did you use the Head executable on your own scripts?

Would you mind linking us to or posting your...


function GameConnection::onConnectRequest( %client, %netAddress, %name )


and


{function GameConnection::onConnect( %client, %name )


functions to see if there is a glitch there?

#3 Those functions are in the common directory which I haven't touched at all. I cant test right now but I suspect the problem is that when a client is rejected it calls the script function GameConnection::onDrop (where the player count is decreased) but it never called GameConnection::onConnect (where the player count is increased) because the connection wasn't completed.

#4 Looks like I was right, I verified it in release_1_2_1 and managed to fix it. Unfortunately I still have a problem because in the older version that i'm using it actually freezes the other clients.

#5 Could you post the fix?

#6 Ok this is a hack but hey thats how i work :(

Changes are in bold.
... = code ommitted to save space

example/common/server/clientConnection.cs

function GameConnection::onConnectRequest( %client, %netAddress, %name )
{
   echo("Connect request from: " @ %netAddress);

   [b]%client.rejected = 1;[/b]
	
   if($Server::PlayerCount >= $pref::Server::MaxPlayers)
      return "CR_SERVERFULL";
   return "";
}

...

function GameConnection::onConnect( %client, %name )
{
    [b]%client.rejected = 0;[/b]

      ...
}

...

function GameConnection::onDrop(%client, %reason)
{
    [b]if(%client.rejected = 0)
    {[/b]
        %client.onClientLeaveGame();

        ....

    [b]}[/b]
}

A more elegant solution would be to use GameConnection::onConnectRequestRejected which i see references to in the code, but i havent figured it out yet.

#7 Ok first of all this should be in the bugs forum :p

The problem here is that the onDrop is making the stupid assumption that the client was connected, which isn't always true. This is a logic error rather than a code bug.

The easiest way to fix it is to keep track of if the client connects or not.

In GameConnection::onConnect

// Save client preferences on the connection object for later use.
   %client.gender = "Male";
   %client.armor = "Light";
   %client.race = "Human";
   %client.skin = addTaggedString( "base" );
   %client.setPlayerName(%name);
   %client.score = 0;
[b]
   //Add this code
   %client.connected = 1;[/b]

In GameConnection::onDrop
Replace:

$Server::PlayerCount--;

With

if (%client.connected)
   {
        $Server::PlayerCount--;
        %client.connected = 0;
   }

#8 Aha I found what was crashing it, once again proving that you dont need to know what you're doing in order to fix stuff! For future reference for anyone using an old version of the engine:
in void NetConnection::onRemove(), change this:

if(mNextConnection)
      mNextConnection->mPrevConnection = mPrevConnection;
   if(mPrevConnection)
      mPrevConnection->mNextConnection = mNextConnection;
   else
      mConnectionList = mNextConnection;

to this:

if(mNextConnection)
      mNextConnection->mPrevConnection = mPrevConnection;
   if(mPrevConnection)
      mPrevConnection->mNextConnection = mNextConnection;
   if(mConnectionList == this)
      mConnectionList = mNextConnection;

#9 Eric, I see what you were doing now(I think). You were logging into the server with TWO clients, not one. That's where I got confused. I thought you meant if you started a server with a 1 player limit that it would automatically reject you when you tried to log in. Now that I see exactly what you were doing, I can see where the bug is, lol.



@ John. Try not to take this wrong, cuz I have nothing against you at all, but I noticed you have a bad habit of using more code than you need quite often.

Take this for example....

if(%client.connected)
{
        $Server::PlayerCount--;
        %client.connected = 0;   
}

"%client.connected = 0;" is completely unneccesary because at that point in your code, there is no doubt that the client is dropping, and that every variable will be deleted no matter what they are.

Also both of you are overcomplicating your fixes as well. The problem here is that a player is getting subtracted without getting added, so just add him. Why waste all the lines of code on "%client.rejected" or "%client.connected" when you could just do this....

function GameConnection::onConnectRequest( %client, %netAddress, %name )
{
      if($Server::PlayerCount >= $pref::Server::MaxPlayers)
      {
             $Server::PlayerCount++;

             return "CR_SERVERFULL";
      }
      return "";
}

Using the methods you guys used, each and every client, rejected or not, will be adjusted and rechecked by extra code that is uneeded. Using the above method, none of the players need to be modified. As you can see, if the client gets rejected, the server player count is increased by one(because he is still getting in for a brief second) and then when he drops, the server will subtract him again and all will be fine. IMO it's a much cleaner and safer solution.

Now, while the above would certainly function perfectly, I prefer to use even more decisive and efficient action. Reference the following example...

function GameConnection::onConnectRequest( %client, %netAddress, %name )
{
      if($Server::PlayerCount >= $pref::Server::MaxPlayers)
      {
             %client.schedule(10, "delete();");

             return "CR_SERVERFULL";
      }
      return "";
}

Even if I had not misunderstood Erics method of replicating the glitch, I still would have never gotten it to work for me because using my method the client is just flat out deleted and never even gets to the "GameConnection::onDrop" Function at all. Why waste all the time processing the client's variables and handling a drop event(Which in my case involves notifying the other clients any time a player drops). Instead of using extra code to compensate for players that are not going to be a part of the game, I just prefer to use one simple "Delete" to get rid of them and move on with the game.


Here is my connect request function. Notice I use multiple checks on the connect request, so I just check the conditions and then either allow the player in, or get rid of him immediately without wasting anymore CPU time....

function GameConnection::onConnectRequest(%client, %netAddress, %name, %type)
{
      if(BanList::isBanned(%name, %netAddress))
      {
            %reject = "CR_YOUAREBANNED";
      }

      if(%type $= "Spec" && $Server::SpectatorCount >= $Pref::Server::MaxSpectators)
      {
            %reject = "CR_SPECTATORSFULL";
      }

      if(%type !$= "Spec" && $Server::PlayerCount >= $Pref::Server::MaxPlayers)
      {
            %reject = "CR_SERVERFULL";
      }

      if(%reject)
      {
            %client.schedule(10, "delete();");
            return %reject;
      }

      return "";
}

So far this has worked perfectly for me. And IMO is the most efficient way to handle client logins or rejections.

#10 That's what I've done:

function GameConnection::onDrop(%client, %reason)
   if (!ClientGroup.isMember(%client))
      return;

   ...

#11 Gonzo:

Isn't your code longer than Johns anyway? :)
He just added like 4 lines.

#12 Gonzo the main difference between your code and mine is that you are specifically dealign with the problem of "Player is refused connection because server is full and the player count is messed up" while i'm dealing with it more generically (only increment the player count if the player actually connects and only decrement the player count if the player was actually connected). Generally generic is better. Less heavy handed :) But either way works. Whatever suits your fancy :)

Also

Quote:
"%client.connected = 0;" is completely unneccesary because at that point in your code, there is no doubt that the client is dropping, and that every variable will be deleted no matter what they are.

Maybe, but its just good programming practice. Assuming a variable will be set to 0 or removed is bad practice, though in the case of TorqueScript its a pretty safe assumption.

#13 We C++ programmers like to make sure all our variables are initialized. It makes it a lot less likely our assumptions will be violated.

Of course, TS already inits all for you, so it's not as big a deal.

#14 Yiehaa, sounds like we'r havin' ourself some OK Corall action in here, gunslingin code-samples an'all ;)

Settle down now ya' hear !!

:-)

#15

Quote:Gonzo:

Isn't your code longer than Johns anyway? :)
He just added like 4 lines.

Well, if 1 is greater than 4, then yes, my code is longer.

#16

Quote:Maybe, but its just good programming practice. Assuming a variable will be set to 0 or removed is bad practice, though in the case of TorqueScript its a pretty safe assumption.

If I can't "assume" that a deleted variable will actually be deleted, then how can you "assume" that a zeroed variable will actually be zeroed?


Besides, I would prefer to find out that the delete is failing rather than to assume it was working because I added in redundant code just in case it didn't work.

#17 This fix is in HEAD, hurray!e

#18 I am just wondering where/how you fixed this in head. I just compared my current build with Torque 1.4 and could not find this fix in there anywhere. I know I must be missing it somewhere, but I can't find it for the life of me...

Any help you could provide would be great!

Thanks in advance,

Stephane

Edit: Just in case you were wondering, I just tested out Marcelo's solution and it works great! It also seems to be the most elegant of the above solutions...

#19 @Stephane: Keep in mind that Ben Garney's post was more than 17 months ago...it's been fixed for a LOOONNG time :)

#20 Hehe... thanks Stephen, that means that the fix should probably be in Torque 1.4, does't it? I don't see it anywhere... I'm pretty sure its not in Torque 1.3 either, as far as I can tell, and I think Ben's post was made pre-1.3 Torque...

If you don't remember where the fix was made, might you have a CVS log from that time that might explain more? Or should the fix not be re-applied... its a pretty major bug...

Anyway, thanks for the quick response!