search

minetest-mods / player_monoids

Monoidally-managed Minetest state modifications
Other
12 stars 8 forks source link

check player object is valid before apply anything, on call backs #7

Closed mckaygerhard closed 2 years ago

mckaygerhard commented 2 years ago

the error was never really solved.. its just that is not happened so much anymore.. but still present in some servers..

adga137 commented 2 years ago

why so many complains event note that we need those contributions.. i cannot see anything wrong here....

SmallJoker commented 2 years ago

@adga137 Just because "it works" does not necessary mean that it's good code. If player needs to be checked for validity in callbacks executed by Minetest itself, the problem is very likely elsewhere. Fixing it in this mod would be a hacky workaround.

mckaygerhard commented 2 years ago

@SmallJoker this hacky workaround is made in many other several mods.. by example in nssm of tenplus1 stop placing excuses

SmallJoker commented 2 years ago

There's no reason to work around ancient Minetest bugs when they're long time fixed. Update Minetest instead.

raymoo commented 2 years ago

How can I reproduce the bug? I don't think your fix helps with the linked issue, because it looks like the problem is invalid non-nil player references.

EDIT: By linked issue I mean minetest/minetest#8452. minetest-mods/irc#69 looks unrelated, because user is not meant to be a player object (at least I think, since there's no "nick" key for ObjectRef).

mckaygerhard commented 2 years ago

How can I reproduce the bug? I don't think your fix helps with the linked issue, because it looks like the problem is invalid non-nil player references.

EDIT: By linked issue I mean minetest/minetest#8452. minetest-mods/irc#69 looks unrelated, because user is not meant to be a player object (at least I think, since there's no "nick" key for ObjectRef).

Sequence of events as nearly as I can tell:

Example code

I'm using code like this in a lot of places to determine if it is safe to run some function:

local player = minetest.get_player_by_name(pname)
if not player or not player:is_player() then
    return -- Or abort or whatever.
end
raymoo commented 2 years ago

I mean, how did you reproduce the bug for player_monoids specifically? I tried doing /test_monoids on a local server and then leaving, but the server was fine. I'm not sure what else would cause it.

mckaygerhard commented 2 years ago

I mean, how did you reproduce the bug for player_monoids specifically? I tried doing /test_monoids on a local server and then leaving, but the server was fine. I'm not sure what else would cause it.

its happened in same way but cases are in low speed connection for some players.. in my case ihave level3 so i cant but my players in the sub nasa most wanted server reports so much the problem and u see it in the console..

as i discuss with tenplus1 this inproductive way to find excuses do not help serverowners

raymoo commented 2 years ago

Peppering the code with random checks is just cruft if it's not something that can happen, which is why I want to confirm an actual case. I'm not taking your word for it because your original fix and being wrong about https://github.com/minetest-mods/irc/pull/69 being an instance of the bug suggests you don't understand the error or when it can happen. If you can link some of the server logs showing the bug (even for another mod, as long as the mod code is available) then that would help.

I fixed the test monoid (08bc018) not to use invalid player references though since that could easily happen from the steps I mentioned for /test_monoids (An invalid player reference doesn't result in crashing there but it's still bad to have in an example). This is just a programming bug from reusing the reference in the minetest.after callback, not a result of minetest providing invalid references.

mckaygerhard commented 2 years ago

Peppering the code with random checks is just cruft if it's not something that can happen, which is why I want to confirm an actual case. I'm not taking your word for it because your original fix and being wrong about minetest-mods/irc#69 being an instance of the bug suggests you don't understand the error or when it can happen. If you can link some of the server logs showing the bug (even for another mod, as long as the mod code is available) then that would help.

I fixed the test monoid (08bc018) not to use invalid player references though since that could easily happen from the steps I mentioned for /test_monoids (An invalid player reference doesn't result in crashing there but it's still bad to have in an example). This is just a programming bug from reusing the reference in the minetest.after callback, not a result of minetest providing invalid references.

good explanation.. i not focus on those explanations cos that bug are so famous in minetest and still happened.. but as we can see minetest is just a base to make money from multicraft.. as issue https://github.com/minetest-mods/irc/pull/69#issuecomment-1076831630 pointed stupid ovbious eplanations are waste of time.. people like @SmallJoker or @monte48 try to make the development of minetest slow and stupid to property benefice multicraft

raymoo commented 2 years ago

for my response to https://github.com/minetest-mods/irc/pull/69 see https://github.com/minetest-mods/player_monoids/pull/7#issuecomment-1051730007