Add `pcall` around wielder calls

[BuckarooBanzay]

Issues like this: https://github.com/mt-mods/signs_lib/issues/7 could be prevented simply by adding a pcall around the deployer/node-breaker calls.

I'm aware this won't fix the root-problems but it would prevent some game-interrupting crashes. The errors should also be logged and raised somewhere too.

Ideas/Thoughts?

[S-S-X]

Very clear error messages with information about mod and function being called, stacktrace would be good. This would be to allow finding mods that do not work well with pipeworks fake player.

I think this is important because pcall can also cause problems when mod code crashes, pcall just makes sure that server wont crash but it is still exception and might need special recovery. Adding pcall without this information will essentially hide errors, it is important from beginning.

Yes, I do think that if enough debug information and clear errors are not added then it would be better to crash server. Pcall is fine but good error logging is must, it is real error situation and can lead to unexpected mod, player or world states.

[OgelGames]

There used to be error handling in monitoring to prevent crashes, I'm not sure why it got removed, but I feel like there was probably some reason for it: https://github.com/minetest-monitoring/monitoring/commit/1a1bda6b4b37a21f3f398e02e760cd50b0e034ef

IMO the correct way to fix it would be to implement all the other functions into the fake player, possibly using the __index metamethod to cover unknown stuff.

[S-S-X]

IMO the correct way to fix it would be to implement all the other functions into the fake player

Problem with this is that most mods should not handle fake players but they attempt because mods do not take into account any other situation than real player. Also remember that you'll also need to patch mintest core Lua API to make it "work" or problem stays.

I do think pcall is really only way to catch problem situations and make other mods handle situation correctly. Some methods could be implemented but implementing complete API is not limited to player API but also includes all Minetest Lua API methods that expect player (userdata) instance as argument, sounds is one common for example.

Then player metadata API comes into play, there you'd have to make decision how to implement it. Should metadata be stored to real player account (mods can for example expect that some metadata is available if one field is present) or thrown away and always return... well, what?

[fluxionary]

I've seen this issue crash something like a dozen mods at this point. Most recently, the trigger was a callback in petz when someone put food into a deployer. I was just about to come over here and propose both of the things Buckaroo already has.

Getting rid of the fake player entirely might also be a good idea. Avoid using API features that need a player argument, or pass nil if you have to. I'm sure that some mods would still not check whether player was nil, but that's at least part of the documented API in places:

• https://github.com/minetest/minetest/blob/9aabd911eb57a5ddef72dd9b7c96f5cca1bd258e/doc/lua_api.txt#L7709
• https://github.com/minetest/minetest/blob/9aabd911eb57a5ddef72dd9b7c96f5cca1bd258e/doc/lua_api.txt#L7716
• etc.

Yet another idea might be to create a whitelist of items that can be put into the "wielder" devices, so that testing could be done before any players get a chance to do it for you.

Then player metadata API comes into play, there you'd have to make decision how to implement it.

The way to handle this is to discard any attempt to set a value in the "metadata" and log it. Return default values for all keys.

[S-S-X]

Avoid using API features that need a player argument

Well you cannot really decide what API features other mods need and pipeworks should not be limiting that. If assuming that other mods will take pipeworks into account then there's nothing to fix but cannot really assume that. Well... you could temporarily patch core API and some functions could be just noop but is that good idea?

or pass nil if you have to

Unfortunately there's a problem with this: Passing nil instead of player would break a lot of things and would for sure cause more harm than good, probably not really option at all. It would even break pipeworks very own devices in protected areas.

another idea might be to create a whitelist of items that can be put into the "wielder" devices

In theory that would probably work best... but list of items and nodes is huge already and after that overrides comes into play... Basically blacklist would be a lot shorter but still incomplete and whitelist would be huge and still wont guarantee anything. It will be really hard to keep up to date, also situation probably would not improve much anyway.

Pipeworks could allow marking compatible/incompatible items but sadly that also would not improve situation much because making sure that items are and actually stay compatible will be hard (as we've seen already with common standard item crashes like pick axes in node breaker) + I don't really like idea of restricting creativity by blacklisting items.

[fluxionary]

I'm going to rephrase the problem:

Pipeworks is currently breaking the published API by passing a parameter that acts like a player in some cases, but crashes the server in others. the current solution to this is that every other mod in the ecosystem has to test to see whether the object they've got is a pipeworks fake player. That is incredibly inconsiderate of pipeworks, and creates a whole lot of headaches for everyone who uses this mod. I'm trying to work on other things, and that gets disrupted when I (or another admin) has to hot-patch yet another mod because it doesn't expect a fake player. I honestly don't care how it gets fixed, but abiding by the current API spec (which in some cases requires reading the minetest code) seems to be a good starting point.

[S-S-X]

I honestly don't care how it gets fixed,

I do because methods that will limit existing use cases and compatibility were suggested already, additionally methods that will only partially fix it and still cause crashes were suggested already.

but abiding by the current API spec (which in some cases requires reading the minetest code) seems to be a good starting point.

What I'm trying to say is simply that you cannot achieve that at all without breaking compatibility with many mods or requiring every other mod in the ecosystem to be specifically programmed to take fake player into account.

Only way to keep existing compatibility and to not cause crashes is to either wrap thing in pcall (simple and good) or monkeypatch large part of engine Lua API (not good).

Additionally if more functionality should be implemented or not is completely different question, implementing full player API will not fix this issue completely (it might make few more mods work but not all). Therefore it is kind of off topic here and should be done to add features but not as an attempt to fix this issue.

[BuckarooBanzay]

I do think pcall is really only way to catch problem situations and make other mods handle situation correctly.

closing this because of above reasoning ^

Pipeworks is currently breaking the published API by passing a parameter that acts like a player in some cases, but crashes the server in others. the current solution to this is that every other mod in the ecosystem has to test to see whether the object they've got is a pipeworks fake player. That is incredibly inconsiderate of pipeworks, and creates a whole lot of headaches for everyone who uses this mod.

@fluxionary feel free to open another issue for this if required

[fluxionary]

i think #98 makes this unnecessary, though we'll have to occasionally update that code when things get added to the player object API.

[S-S-X]

i think https://github.com/mt-mods/pipeworks/pull/98 makes this unnecessary, though we'll have to occasionally update that code when things get added to the player object API.

Having fully implemented player API is very nice but good to remember it wont make it behave like a real player.

In other words, it is only helping with few very simple and straightforward crashes but more importantly it is adding new features for fake player. To actually say it'd be able to handle situations where many of the now fixed past crashes happened you'd have to actually also patch core engine lua api, wouldn't recommend going there.