ikkentim / SampSharp

A framework for writing game modes for SA-MP in C#. SA-MP is a free Massively Multiplayer Online game mod for the PC version of Rockstar Games Grand Theft Auto: San Andreas.
https://sampsharp.net
Apache License 2.0
209 stars 43 forks source link

BasePlayer.GetAll method does not return the expected value #372

Closed MrDave1999 closed 2 years ago

MrDave1999 commented 3 years ago

The problem is simple, for example, if there are 4 players on the server and then one disconnects, BasePlayer.GetAll returns all 4 instances, instead of 3. That is, it is taking the instance of the player that disconnected.

This generated an exception (NullPointerException) in a system that I was doing.

What should I do about it?

ikkentim commented 3 years ago

Could you give an example of when this happend? Possibly you used BasePlayer.GetAll before the player disconnected, and got the NullReferenceException after an await (allowing the player to have disconnected).

When a player disconnects, sampsharp removes the player from the player pool, thus GetAll should not be returning the disconnected player.

MrDave1999 commented 3 years ago

This is the code where I generate that exception:

[Command("users", Shortcut = "users")]
private static void UsersList(Player player)
{
    var users_alpha = new List<Player>(GameMode.TeamAlpha.Members);
    var users_beta = new List<Player>(GameMode.TeamBeta.Members);
    var users = new TablistDialog(
    $"{GameMode.TeamAlpha.OtherColor}Alpha: {GameMode.TeamAlpha.Members} {GameMode.TeamBeta.OtherColor}Beta: {GameMode.TeamBeta.Members}",
    new[] {
         "Id",
            "Name",
            "Kills",
            "Deaths"
        }, "Cerrar", "");
    foreach (Player player1 in BasePlayer.GetAll<Player>())
    {
        if (player1.Team == BasePlayer.NoTeam) 
            continue;
        (player1.Team == (int)TeamID.Alpha ? users_alpha : users_beta).Add(player1);
    }
    Console.WriteLine("Players: " + x);
    users_alpha.Sort((a, b) => b.Kills - a.Kills);
    users_beta.Sort((a, b) => b.Kills - a.Kills);
    foreach (Player player1 in users_alpha)
        users.Add(player1.ToString());

    if(users_alpha.Count > 0)
        users.Add(" ", " ", " ", " ");

    foreach (Player player1 in users_beta)
        users.Add(player1.ToString()); //NullPointerException here.
    users.Show(player);
} 

If, for example, there are 4 Team Alpha users and 4 Team Beta users and then a Team Beta member disconnects, it throws an exception when using the command.

It does not generate an exception again when I put at the end of OnPlayerDisconnected a:

player.Dispose();
CiprianN23 commented 3 years ago

I think better use BasePlayer.All instead as is the collection as array, though GetAll guess is a issue

ikkentim commented 3 years ago

BasePlayer.GetAll uses BasePlayer.All... It should work, but I'll try and verify it when I can.

ikkentim commented 3 years ago

Hello,

I cannot reproduce this issue with the following code:

[Command]
public static void ListPlayersCommand(BasePlayer player)
{
    // Issue 372 test
    player.SendClientMessage($"Players available: {BasePlayer.GetAll<BasePlayer>().Count()}");
    try
    {
        foreach (var p in BasePlayer.GetAll<BasePlayer>())
        {
            player.SendClientMessage("Player: " + p.Name);
        }
    }
    catch (Exception e)
    {
        player.SendClientMessage(Color.Red, e.Message);
    }
}

// Player A connects...
// Player B connects...
// Player A types /listplayers : 2 players are listed
// Player B disconnects
// Player A types /listplayers : 1 player is listed

The player is disposed by the OnCleanup handler in the BasePlayer class.

I'm guessing you either:

Let me know if you have any further questions.

MrDave1999 commented 3 years ago

Hi, Goodnight! I have not overwritten the OnCleanUp method in either the Player class or the GameMode class. The only thing I have in the PlayerController class, is the following:

public class PlayerController : BasePlayerController
{
      public override void RegisterTypes()
      {
           Player.Register<Player>();
      }
}

And in the GameMode class, I have the following overridden method:

protected override void LoadControllers(ControllerCollection controllers)
{
         base.LoadControllers(controllers);
         controllers.Override(new PlayerController());
}

I tried your test code and it gave me the same result. The problem is when player A disconnects. Player A instance has yet to be dispose. It's very weird. I did another test and if the player does not spawn (that is, he stays in the class selection), the instance player A is deleted.

ikkentim commented 3 years ago

If you sent me your code, I can take a look and see where it goes wrong...

Also, I think if an exception occurs during OnPlayerDisconnect, the player might not be disposed properly, I could improve on that

MrDave1999 commented 3 years ago

It is that I tested your code in an empty project and it follows the same problem. The test code I used was: https://pastebin.com/JC1hFjq4

If player "jose" connects and writes /listplayers, the result is:

Players available: 1
Player: jose

If player "pepe" connects and writes /listplayers, the result is:

Players available: 2
Player: jose
Player: pepe

Now, if the player "jose" disconnects and after a moment "pepe" writes /listplayers, the result is:

Players available: 2
Player:
Player: pepe

I don't understand why that comes out, the instance is still "alive".

ikkentim commented 3 years ago

Are you using any filterscripts or plugins? I used exactly your code and it works just fine.

If any of your filterscripts are returning '0' in OnPlayerDisconnect, SampSharp won't function properly.

image

MrDave1999 commented 3 years ago

In the test code I don't run any FS and plugins (except for SampSharp.dll). If the "player1" disconnects, it doesn't show the expected result: img Do the same test please. I do not know what's happening..

ikkentim commented 3 years ago

I did... are you using the latest version of the plugin and sampsharp packages? Are no errors logged in the SampSharp gamemode console?

image

MrDave1999 commented 3 years ago

I am using the SampSharp 0.9.3 plugin and I use the following packages: SampSharp.GameMode (0.9.1), SampSharp.Core (0.9.3) and SampSharp.Entities (0.9.3). I do not understand what the problem is ..

PD: No error appears on the console.

ikkentim commented 3 years ago

Could you try this with the latest (currently in development) version of SampSharp?

You can download it here:

plugin: https://github.com/ikkentim/SampSharp/suites/1643076336/artifacts/30431209

libraries: https://github.com/ikkentim/SampSharp/suites/1652524853/artifacts/30691085

MrDave1999 commented 3 years ago

Tried it, it keeps giving me the same wrong result. What result do you get?

CiprianN23 commented 3 years ago

You said you are using these 3 packges inside your gamemode, do you have all of them installed in your project right now?

SampSharp.GameMode (0.9.1), SampSharp.Core (0.9.3) and SampSharp.Entities (0.9.3)

Is there a posibility of having .GameMode and .Entities in same project to produce this issue? Or maybe having .Core installed even if both .GameMode and .Entities are having already has dependecy

Edit: Just a theory though

ikkentim commented 3 years ago

Good catch Ciprian,

It isn't right that you are using both SampSharp.GameMode and SampSharp.Entities. Please remove one of them (you should probably remove SampSharp.Entities)

MrDave1999 commented 3 years ago

I also did tests using only .GameMode and .Core and still the problem persists.

ikkentim commented 3 years ago

In that case I'm using the same plugin, same code and are doing exactly the same test... I don't see how you'd get different results...

What sa-mp server version are you using? And maybe share your server.cfg for good measure...

MrDave1999 commented 3 years ago

You know, the samp server version I was using was 0.3.7-R1 ...

I downloaded the sampserver 0.3.7-R2 but the problem still persists. What a fart with this, I do not understand why that comes out ...

The server.cfg I use is:

echo Executing Server Config...
lanmode 0
rcon_password 1234
maxplayers 50
port 7777
hostname SA-MP 0.3 Server
gamemode0 empty 1
gamemode Test\Test\bin\Debug\netcoreapp3.0\Test.dll
coreclr runtimes
filterscripts 
announce 0
chatlogging 0
weburl www.sa-mp.com
onfoot_rate 40
incar_rate 40
weapon_rate 40
stream_distance 300.0
stream_rate 1000
maxnpc 0
logtimeformat [%H:%M:%S]
language English
plugins SampSharp

In the test use the packages .GameMode (0.9.1) and .Core (0.9.3).

PD: I forgot to load the FS empty.amx, although I'm not sure if that has to do with the problem ... Same thing I tried again but it still gives the same problem.

MrDave1999 commented 3 years ago

I have not found any solution to this problem. The only thing I could think of was to create my own collection where I save the instances of each player. Practically this would be a "temporary" solution. I will still leave this "issue" open in case anyone knows how to solve it.

KirillBorunov commented 2 years ago

I copy here the info from discord:

I think I found some clues on what is going on The bug is not happening always, it seems to happen when some lag is present, or maybe other variables that I don't know.

There are 2 cases, first I will tell the Case 1: To reproduce we need at leas 2 players, and the players need to be near each other, they should be visible to each other. Then put this code in the GameMode:

        protected override void OnPlayerConnected(BasePlayer player, EventArgs e)
        {
            Console.WriteLine($"OnPlayerConnected ID = {player.Id}");
            base.OnPlayerConnected(player, e);
            Console.WriteLine($"BasePlayer.All.Count = {BasePlayer.All.Count()}");
        }
        protected override void OnPlayerDisconnected(BasePlayer player, DisconnectEventArgs e)
        {
            Console.WriteLine($"OnPlayerDisconnected ID = {player.Id}");
            base.OnPlayerDisconnected(player, e);
            Console.WriteLine($"BasePlayer.All.Count = {BasePlayer.All.Count()}");
        }
        protected override void OnPlayerCleanup(BasePlayer player, DisconnectEventArgs e)
        {
            Console.WriteLine($"OnPlayerCleanup ID = {player.Id}");
            base.OnPlayerCleanup(player, e);
            Console.WriteLine($"BasePlayer.All.Count = {BasePlayer.All.Count()}");
        }
        protected override void OnPlayerStreamOut(BasePlayer player, PlayerEventArgs e)
        {
            Console.WriteLine($"OnPlayerStreamOut ID = {player.Id}");
            base.OnPlayerStreamOut(player, e);
            Console.WriteLine($"BasePlayer.All.Count = {BasePlayer.All.Count()}");
        }

Then disconnect one of the players. In the console you will see this when the bug happens:

OnPlayerDisconnected ID=0
[part] Kirill_Taburetkin has left the server (0:2)
BasePlayer.All.Count = 2
OnPlayerCleanup ID=0
BasePlayer.All.Count = 1
OnPlayerStreamOut ID=0
BasePlayer.All.Count = 2

Another try:

OnPlayerDisconnected ID=0
BasePlayer.All.Count = 2
OnPlayerCleanup ID=0
BasePlayer.All.Count = 1
[part] Kirill_Taburetkin has left the server (0:2)
OnPlayerStreamOut ID=0
BasePlayer.All.Count = 2

As you can see, some times the OnPlayerStreamOut event is arriving AFTER the disconnected&cleanup . And in the callback handler in SampSharp BaseMode.callbacks.cs line 408 we can see this:

        [Callback]
        internal bool OnPlayerStreamOut(int playerid, int forplayerid)
        {
            OnPlayerStreamOut(BasePlayer.FindOrCreate(playerid),
                new PlayerEventArgs(BasePlayer.FindOrCreate(forplayerid)));

            return true;
        }

Note the FindOrCreate(), this is the culpit, this function is creating a phantom player.

For now doing this seems to workaround the problem in my tests (without modify the SampSharp code):

        protected override void OnPlayerStreamOut(BasePlayer player, PlayerEventArgs e)
        {
            Console.WriteLine($"OnPlayerStreamOut ID = {player.Id}");
            if (!player.IsConnected) {
                player.Dispose();
            }
            else {
                base.OnPlayerStreamOut(player, e);
            }
            Console.WriteLine($"BasePlayer.All.Count = {BasePlayer.All.Count()}");
        }

After this fix you will see the counter now is 1 as it should be:

OnPlayerDisconnected ID=0
[part] Alex_Forver has left the server (0:2)
BasePlayer.All.Count = 2
OnPlayerCleanup ID=0
BasePlayer.All.Count = 1
OnPlayerStreamOut ID=0
BasePlayer.All.Count = 1

But maybe this can happen with other events, need more testing...