FPtje / DarkRP

DarkRP, a non-serious roleplay gamemode for Garry's Mod.
https://darkrp.miraheze.org
MIT License
463 stars 709 forks source link

4 Errors #2640

Closed mcNuggets1 closed 7 years ago

mcNuggets1 commented 7 years ago

Description of the bug

Its a lua error

How to make the bug happen

Lua errors

[ERROR] gamemodes/darkrp/gamemode/modules/money/sv_money.lua:44: Tried to use a NULL entity!

  1. SetPos - [C]:-1

    1. createMoneyBag - gamemodes/darkrp/gamemode/modules/money/sv_money.lua:44
    2. unknown - gamemodes/darkrp/gamemode/modules/base/sv_gamemode_functions.lua:434
      1. Kill - [C]:-1
      2. Call - gamemodes/base/gamemode/player.lua:369
      3. PlayerSelectSpawn - gamemodes/base/gamemode/player.lua:479
      4. unknown - gamemodes/darkrp/gamemode/modules/base/sv_gamemode_functions.lua:603
        1. Spawn - [C]:-1
        2. unknown - gamemodes/base/gamemode/player.lua:115

    [ERROR] gamemodes/darkrp/gamemode/modules/fpp/pp/server/antispam.lua:120: attempt to index local 'ent' (a nil value)

    1. IsEmpty - gamemodes/darkrp/gamemode/modules/fpp/pp/server/antispam.lua:120
    2. DoPlayerEntitySpawn - gamemodes/darkrp/gamemode/modules/fpp/pp/server/antispam.lua:163
    3. GMODSpawnProp - gamemodes/sandbox/gamemode/commands.lua:169
      1. unknown - gamemodes/sandbox/gamemode/commands.lua:27
      2. unknown - lua/includes/modules/concommand.lua:54

[ERROR] gamemodes/darkrp/entities/entities/chatindicator/init.lua:29: Tried to use a NULL entity!

  1. SetPos - [C]:-1
    1. func - gamemodes/darkrp/entities/entities/chatindicator/init.lua:29
    2. unknown - lua/includes/extensions/net.lua:32

[ERROR] gamemodes/darkrp/gamemode/modules/hungermod/sv_hungermod.lua:74: attempt to call method 'Setowning_ent' (a nil value)

  1. callback - gamemodes/darkrp/gamemode/modules/hungermod/sv_hungermod.lua:74
    1. callback - gamemodes/darkrp/gamemode/modules/chat/sv_chat.lua:17
    2. unknown - gamemodes/darkrp/gamemode/modules/chat/sv_chat.lua:255
      1. unknown - lua/includes/modules/concommand.lua:54

Why the developer of DarkRP is responsible for this issue

his addons

mcNuggets1 commented 7 years ago

Brand new:

[ERROR] gamemodes/darkrp/entities/entities/fadmin_jail/init.lua:26: Tried to use a NULL entity!

  1. SetPos - [C]:-1
    1. unknown - gamemodes/darkrp/entities/entities/fadmin_jail/init.lua:26

[ERROR] gamemodes/base/entities/entities/gmod_hands.lua:30: Tried to use a NULL entity!

  1. DeleteOnRemove - [C]:-1
    1. DoSetup - gamemodes/base/entities/entities/gmod_hands.lua:30
    2. SetupHands - lua/includes/extensions/player.lua:225
      1. Call - gamemodes/darkrp/gamemode/modules/base/sv_gamemode_functions.lua:527
      2. func - gamemodes/darkrp/gamemode/modules/base/sv_jobmodels.lua:18
      3. unknown - lua/includes/extensions/net.lua:32
Bo98 commented 7 years ago

Is your ents.Create broken or something?

mcNuggets1 commented 7 years ago

Not at all, these are Errors are very rare.

They likely appear on a always full server once a week.

Bo98 commented 7 years ago

Every one of these are caused by ents.Create returning a NULL entity.

My guess is it possibly hitting the entity limit. Does anything else print in the console just before these errors happen?

Kefta commented 7 years ago

Validity checks should be done on entities made by ents.Create, anyway

FPtje commented 7 years ago

Are you guys starting to understand how much I hate the concept of null? You can never trust anything not to be null.

On 1 Mar 2017 09:05, "Collin (code_gs)" notifications@github.com wrote:

Validity checks should be done on entities made by ents.Create, anyway

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/FPtje/DarkRP/issues/2640#issuecomment-283272232, or mute the thread https://github.com/notifications/unsubscribe-auth/ABJXXkm6P1asHMfDdWhYp3DnoqpYmThAks5rhSbhgaJpZM4MOXOz .

Kefta commented 7 years ago

It would be great if "if (!NULL) then" worked in Lua, but I guess it does have the benefit of being able to run ent:IsValid on it to abstract all entities validity checks. Anyway, you CAN trust entities to not be NULL; here's a list of rules that I made for when to do validity checks:

  1. Entity that is networked through the net library.
  2. Entities created or accessed with the ents.Create(), Entity(), Player(), or LocalPlayer() functions.
  3. Entities returned from other entity functions (GetParent, GetActiveWeapon, etc.).
  4. Stored EHandles after time has passed. This can be from an async callback, hook, timer, or just storing the entity for later use after a set amount of time.
  5. SWEP.Holster can provide a NULL weapon if the weapon is being removed, or the player is switching to NULL.

NOTE: Trace result entities will always be valid if tr.Hit == true/tr.Fraction != 1 (same thing). Otherwise, they will be NULL. Note that worldspawn:IsValid() returns false, so if you are wanting the world to be considered a valid entity, you can check SomeEntity == NULL instead.

Hooks and callbacks that provide entities will always provide valid ones, with PreDrawViewModel being the exception due to a bug(?), but that provides nil instead of NULL. Also, you should never have to use util.IsValid/IsValid on entities if you structure your code properly to handle validity cases from the start.

MrBrax commented 7 years ago

why not !IsValid(ent)? or am i misunderstanding this?

FPtje commented 7 years ago

How would you handle validity cases from the start while also never using util.IsValid/IsValid on entities?

Also, your rule list of when to do validity checks is very, very GMod specific. It doesn't translate to other languages with concepts of NULL. Unless your generic approach is to investigate the API of the languages and generate a new rule list of when to check for NULL.

Kefta commented 7 years ago

@MrBrax IsValid(ent) checks if the entity is nil, has an IsValid method, and then runs ent:IsValid(). Although it makes sense for panels since they can be nil before initialisation, if you structure your code to not allow "nil" to even be considered an entity, IsValid() becomes wasteful since entities will always have an IsValid method.

@FPtje Using the rules I listed. Here's some code excerpts from my weapons that properly handle validity without having to ever having to use IsValid() since entities will never be "nil." I still use "== NULL" in my code in some places due to an old habit that I need to write a regex script to correct, but it esentially acts the same as ent:IsValid() in the cases I'm dealing with. http://pastebin.com/a3ew9RWg

And you are correct. This is GMod specific since GLua is different in that NULL is not an empty data type or invalid pointer -- it's a side-effect of being based on a C++ engine. Other languages have their own specific cases of how to properly check for nullptrs, empty data sets, etc. NULL/nullptr makes a lot of sense in C/C++, but has caveats when being transferred to a scripting language.

MrBrax commented 7 years ago

but isn't the issue that an entity is null when trying to access a function on it? i see no reason not to use IsValid there

Kefta commented 7 years ago

@MrBrax IsValid(ent) != ent:IsValid(). NULL:IsValid() is a valid usage of the function and will not error. Fun fact, NULL:IsPlayer(), NULL:IsVehicle(), etc also doesn't error, and so doing IsValid(ent) and ent:IsPlayer() is redundant.

FPtje commented 7 years ago

I always prefer IsValid over ent:IsValid because nil is just the Lua equivalent of null. You can't ever trust anything not to be nil either, unless you know the fucking implementation of the functions. You can see this in @Kefta's rule list, which is a list of interfaces that return nil/NULL (as far as we know at least). Others are apparently known not to return NULL, with of course one exception where it does give nil. That exception, of course, is probably not the only one.

Basically you have two choices: either you always use IsValid for everything, or you find the implementation of every single function you use and prove that they cannot return NULL/nil. Either way that's a pathetic state to be in.

Besides, how am I even supposed to deal with NULL after ents.create? Besides reverting the action, what should I tell the client who's trying to FAdmin jail someone, what should I tell the player of whom the chat bubble can't created?

Sorry, for some fucking reason I can't create entities anymore. I didn't cause this error, and I cannot be sure what the cause is. Maybe you hit the entity limit, maybe someone took a dump on your server causing unexplained behaviour, but shit is fucked now. Try cleaning your server or something?

I am saddled with errors that I didn't cause. I get the blame for shit like entity limits being hit because the failure is silent until the consequences causes DarkRP to throw fucking errors.

Everyone can bet their battered anuses that DarkRP won't be the only addon throwing errors with ents.Create returning NULL. What should we do about that, show infomercials on some global TV channel telling people that only bad coders don't check for NULL after ents.Create, or do we acknowledge the root cause of the issue: ents.Create not Creating ents.

How about I fucking override ents.Create with an IsValid check, throwing a huge ass error about the server being fucked? It would take the blame off me, it would take the blame from literally every scripter who doesn't literally spam their fucking code with validity checks. It would be helpful for server owners, who would better understand the error and solve it by cleaning the server or changing map.

Kefta commented 7 years ago

@FPtje You're treating Source like it's still a hot codebase subject to radical change. I don't blame anyone for using IsValid() for safety since it does require implementation knowledge -- I'm still learning conditions about common functions I never knew before, but nil/NULL are well-defined and different. NULL is a pointer/handle. Entities can become NULL, but nothing can just "become" nil. That's like saying a table can just become a number out of the blue (changing ent.SomeVar to a different type doesn't really count in this case since that's a side-effect of dynamic binding when really we're discussing type morphing here).

And yes, I've tested all entity callbacks and hooks and PreDrawViewModel is the only exception, which doesn't seem to even be intended from Robotboy's post.

GLua just wasn't created with well-defined behaviours, it wasn't made to be that kind of language. There's a lot of grey area that can only be "resolved" by weak induction and testing multiple cases. I don't think it's pathetic to try to find these consistencies -- it can make your code more clear and concise, arguably at the sake of the function being subject to change, but I have never seen a GMod update change something's argument type or definition without it being months with forewarning (Garry's Mod x -> Garry's Mod x+1).

And I wouldn't override ents.Create because you're now fucking directly with other people's scripts and just adding on more validity checks than necessary. Just check if the entity created in YOUR code is valid from it. If not, print out a message saying they've hit the entity limit, or the entity doesn't actually exist at all (they touched core files most likely and deleted an entity they shouldn't have). Valve does the same thing: they print that the entity limit was hit, but they take the extra initiative of starting to delete world entities, which is not necessary in this case.

FPtje commented 7 years ago

but nothing can just "become" nil

with PreDrawViewModel being the exception due to a bug(?)

Things can be nil for the exact same reason they can be null: some computation failing. Variables can change from one type to another. That's just a fact in a dynamically typed language such as Lua. Of course this is usually a bug, but it can happen nonetheless.

My point is simple. The cause of the errors is the SERVER being in an error state. That error is NOT MY FAULT. The particularly shitty result is that this ONE cause of an error state manifests itself in EVERY call of ents.Create. And somehow EVERYONE who uses that function gets the blame for not dealing with an error state they didn't cause because they don't have validity checks.

If anything, attacking the root cause of the issue is the best way to solve this. The server is in an error state when ents.Create returns NULL. That has VERY far reaching consequences, and is to be treated as such. You can't just pretend the issue doesn't exist, have individual actions fail with weird messages to players.

It's when you have to blame a large amount of people that you should reconsider where the blame really lies.

How about this point: I'll get bug reports about people not being able to create jails or other weird shit suddenly not being possible anymore. Instead of a clear error that clearly shows ents.Create failed, people will just say it doesn't work. And unless they've got the discipline to mention that weird message I could send to them, no one will ever find out what's going on. And we'll be worse off.

The game is in an error state. Treat it as such instead of pretending everything's actually OK.

Kefta commented 7 years ago

I'm not pretending it's okay. Hitting the entity limit/the entity not existing isn't your fault. I think you're taking people thinking these errors being DarkRP's fault a bit too personally -- it's a case that Valve accounted for and put in defined behaviour for. The problem is that they did not expect anyone to put a scripting language on top of Source, and Garry decided to return NULL if the entity isn't created -- his choice.

Also, I wouldn't even say the server is in an "error state," it's clearly defined to return NULL if the entity can't be created. It doesn't return garbage data or something. Whether you want to account for that is up to you.

To your point about "Things can be nil for the exact same reason they can be null: some computation failing." -- they're actually completely different. NULL is designed to be an entity. Entities can become NULL naturally without setting anything over time due to them replicating pointer behaviour. Nothing can just become nil unless it is physically set in your code.

PreDrawViewModel passes nil due to an intentional call in the engine. It will still CALL that render frame with a VALID viewmodel, but it will call nil later on as well for some reason that the current developers of Garry's Mod do not know the intention of. That's not an entity becoming nil, it's the intentional passing of nil.

FPtje commented 7 years ago

NULL doesn't exist in pure Lua. nil does. nil performs the duty of NULL in pure Lua. NULL is a C concept, a GMod concept, and it only exists in Lua because it interfaces with C++.

An uninitialised value in Lua is nil. An uninitialised value in C++ is null. When a computation fails in Lua, nil is returned to signify some failure. When a computation fails in C++, null is returned to signify some failure. The only reason why there's both nil and null in Lua is because it's imported from the FFI.

You could theoretically translate all the nulls in Lua to nil, and you wouldn't lose anything of value. The language would be as expressive. Nothing would suddenly become impossible. Nil and null are truly isomorphic.

Anyway, have a vote. I'm going to override ents.Create. Here's your choice:

error would bypass the scripts that do have validity checks to deal with the error situation. errorNoHalt doesn't prevent the other errors from being thrown. People might miss the huge errorNoHalt message and still blame me. They wouldn't with error.

Kefta commented 7 years ago

@FPtje They're conceptually different. NULL represents a pointer, nil is just empty data. They both have their place and act differently. NULL in GMod refers only to entities, nothing more. An entity can't be nil because nil doesn't have data.

If you do override ents.Create, do error and please do not use IsValid(ent) or ent:IsValid() on the return. Check with ent == NULL. Note that it won't work for physics objects created manually, though, since physics object have their own NULL type that doesn't have a keyword/enum in Lua, but I don't think they can be created thru ents.Create.

FPtje commented 7 years ago

some entities aren't valid when they're created

So they're not null but still invalid, but not really because they just need a little time to convince themselves that they're actually valid? What a fucking shit show.

Kefta commented 7 years ago

I'm sorry, I was semi-incorrect before. This does apply to vehicles, but they have their own method ( http://wiki.garrysmod.com/page/Vehicle/IsValidVehicle ) to determine safe usage time. ent:IsValid() checks for NULL and if the entity is worldspawn ( http://wiki.garrysmod.com/page/Entity/IsValid ).

FPtje commented 7 years ago

What a fucking shitshow

MrBrax commented 7 years ago
_G.NULL = nil;
Kefta commented 7 years ago

@MrBrax That would just remove the enum. C++ methods would still return NULL, but now you can't access what it is.

FPtje commented 7 years ago
iso_nil_null = { toNil = function() return nil end, toNull = function() return NULL end }

The nil null isomorphism witness.

Kefta commented 5 years ago

@FPtje I've been thinking about this discussion again recently as I've delved into language design and I thought of a very good reason why nil can't replace NULL: type confusion. TypeID(NULL) == TypeID(Entity(0)), thus any objects that are "transformed" into NULL (really just an internal handle check that still points to the same TValue/Lua object) will still have the same type. Thus you can make the assumption in Lua, as well as most other dynamic programming languages due to their own consequential standards, that the type of a variable's value will not change unless the user does so themselves.

Using NULL objects instead of a centrally "empty" data keyword is also a pretty common design pattern that allows operator behaviour to remain consistent and safe across an API, as opposed to something like MyClass *foo = nullptr; foo->blah(); in C++ (which segfaults) or local obj; obj:blah(); in Lua (which errors - using nil as a NULL object would just throw a "indexing nil variable" error instead of a type-specific error).

Changing an object into nil already isn't possible with the public Lua API, but allowing so would introduce tons of type confusion and uninformative errors since the user won't know if a variable has been altered without their knowledge.

FPtje commented 5 years ago

NULL has a C++ type, nil has a Lua type. You're right in that there would be type confusion. Unlike Lua, you can't go changing the types of stuff in C++, lest you get a compiler error. Null pointers have the type of the class of which they are null. If you have a removed Vehicle, you'll have a null pointer of type Vehicle. Lua, as a dynamically typed language, has nil as a separate thing.

I disagree that it's a "safe" pattern though. I'd never call the null error shit I'm dealing with here "safe" in any way, shape or form. Something about silent failure until the consequences start giving errors. I'll show you safety in a moment. In my vision, NULL and nil are the same concept, but in different languages. Obviously with slightly different implementations because of that. In the end, though, the only real difference in Lua is the meaning given to them.

I could theoretically create my own NULL/nil and call it Nel. Whether I implement it with an empty table or by changing Lua doesn't really matter. I could make Nel mean something like bad floating point calculations, or maybe errors in DarkRP's interface. With me giving some meaning to Nel, you can argue that nil, NULL and Nel aren't in any way interchangeable, because you wouldn't be able to differentiate the different kinds of errors that there are, especially if I were to give Nel its own errors.

I should give some context on my position in this discussion. In my work I've programmed in a super type strict language, one that has done away with concepts of NULL and nil. That language has safety in the type level. There are several implementations of dealing with failure, but they always have the following properties:

This is what I call safety. This perspective has gotten me pissed off with GMod. Imagine this, I've been the developer of DarkRP since late 2008. By far most of the errors I've resolved in it are NULL/nil errors. I've spent hours upon hours on finding the right ways to deal with this, finding the right patterns, discussing with people about defensive programming. Despite that, they STILL come up every now and then. Finding out that ents.Create could in fact shove a middle finger up the anus and actually not do what the name of the function literally says, made me go mad. There's no way I could have seen this coming. The cause of it is beyond my control, yet I get saddled with dozens of places in the code that suddenly turn out to be error prone.

I bet that if you list the NULL/nil errors over time with basically any big Lua project, you'll find some kind of half-life of the amount of reports over time after the last big refactor. A bunch in the beginning, and fewer and fewer over time. I'm convinced that defensive programming can prevent a lot of the obvious cases, but that not even the best programmer in the world can truly create anything big without these errors popping up in the weirdest places. That's not on the skill of the programmer, it's on the language itself.

It's on the language itself because this shit could all have been prevented with different design decisions. Here's some more info: https://www.lucidchart.com/techblog/2015/08/31/the-worst-mistake-of-computer-science/


On another note, the ents.Create override has worked REALLY well. I haven't heard many other people complain about the errors, and the ones who do know that it's an entity limit problem.

Bo98 commented 5 years ago

Yep, it's the direction a lot of programming languges are heading. For example, Swift and Kotlin have risen in the mobile scene recently, with type safety playing a big part. Things can't be null without declaring it as such.