add guard in lua against nil group from dead unit #232

uriba107 · 2023-07-11T13:39:47Z

In 2.8.2 ED has changed the behaviour of :getGroup() for units. and it will return nil for dead units.
https://wiki.hoggit.us/view/DCS_func_getGroup

in current version, you would get an empty group and feed it to the exporter class which throws errors in dcs.log

2023-06-27 12:52:53.246 ERROR   dcs_grpc: Error in event handler: [string "C:\Users\Carmen\Saved Games\DCS.openbeta_server\Scripts\DCS-gRPC\exporters\object.lua"]:49: attempt to index local 'group' (a nil value)
stack traceback:
	[string "C:\Users\Carmen\Saved Games\DCS.openbeta_server\Scripts\DCS-gRPC\exporters\object.lua"]:49: in function 'group'
	[string "C:\Users\Carmen\Saved Games\DCS.openbeta_server\Scripts\DCS-gRPC\exporters\object.lua"]:27: in function <[string "C:\Users\Carmen\Saved Games\DCS.openbeta_server\Scripts\DCS-gRPC\exporters\object.lua"]:19>
	(tail call): ?
	[string "C:\Users\Carmen\Saved Games\DCS.openbeta_server\Scripts\DCS-gRPC\methods\mission.lua"]:43: in function 'typed_exporter'
	[string "C:\Users\Carmen\Saved Games\DCS.openbeta_server\Scripts\DCS-gRPC\methods\mission.lua"]:341: in function <[string "C:\Users\Carmen\Saved Games\DCS.openbeta_server\Scripts\DCS-gRPC\methods\mission.lua"]:63>
	(tail call): ?
	[C]: in function 'xpcall'
	[string "C:\Users\Carmen\Saved Games\DCS.openbeta_server\Scripts\DCS-gRPC\grpc.lua"]:233: in function 'onEvent'
	[string "Scripts/World/EventHandlers.lua"]:13: in function <[string "Scripts/World/EventHandlers.lua"]:11>

The proposed change tests if the incoming group object is indeed a group (based on lua metatable) and return an empty table if it fails

rkusa

Thanks a lot for the PR, highly appreciated!

rkusa · 2023-07-12T15:41:42Z

lua/DCS-gRPC/exporters/object.lua

@@ -45,6 +45,7 @@ GRPC.exporters.rawTransform = function(object)
 end

 GRPC.exporters.group = function(group)
+  if type(group) ~= "table" and getmetatable(group) ~= Group then return {} end


I think returning {} might lead to effects harder to debug than the current error we get. I haven't tested it, but I'd guess this would simply set all fields of the group to their default / undefined value which is probably unexpected in most cases.

Without giving it too much thought (so please feel free to challenge my suggestion), I think the following might be a better solution:

explicitly define the group property as optional.

handle the possibility of getGroup returning nil in the unit export

Wdyt?

I initially set it with return nil, but then I suspected that someone might try and iterate over a table. but you are correct. it will force someone to put some guards on his end if.

I'm unfamiliar grpc, reading your response, it's obvious that returning nil and specifying optional in the proto file is the correct way to go.

I'll make the changes

I really don't like marking the group property as optional as we have now complicated the handling of every client (and will probably break existing clients who are expecting group to always be there) for a, frankly stupid and god-knows-if-ED-are-going-to-do-something-about-it edge-case..

I haven't heard anyone complaining about missing events (that would have these nils and are currently not being published) so my question is.

What is the goal of this change?

Just stop the error messages appearing?

Publishing essential events that are currently not being published and being skipped due to the error?

If the former then this is an overkill solution and we could reduce the error verbosity for it. If the latter then I think I would like to explore others before we make a change that effects every event with a unit in it.

Options:

Create a DeadUnit / UnitWithoutGroup object instead and only use it for those events that need it.

???

Fair point. I wouldn't mind reducing the DeadEvent down to just containing the unit's name instead of a whole unit object. At least in my use-cases, that is all I would need to cleanup state about the units I track in my clients. I'd probably even prefer it over a UnitWithoutGroup.

Also, I wouldn't be one complaining about missing Dead events, as I don't trust ED to fire that reliably anyway. So instead, I just consider a unit dead whenever I fetch information for it and receive a not found error 🤪

@uriba107 So this brings me back to my question: What is the goal of this PR for you? What is your priority?

My goal? Not seeing as many error messages in the log.
There is also a slight chance that once an error pops up dcs event handler will. Not continue processing more functions bound to that event but that might be just a feeling

I think the most compatible (But not backwards compatible) solution here is the DeadUnit approach.

It will get rid of error messages

It will give people listening to events the ability to react on this event without needing to consult a local storage of units, which they would otherwise have to do if we just sent the unit ID. This may be important for simple stats trackers.

I'm currently trying to debug something on my end. I suspect it has something to do with the grpc throwing LUA errors on parse, but I'll need to be very targeted in my testing, so I'll build a simple test mission tonight and do some tests.

as to how it will be resolved eventually, I trust your judgment for choosing the correct path. but I want to first determine if it's a real issue, as I suspect, or purely cosmetic.

Ok,
I'm circling back to this one.

I've got a unit testing mission for my project. I had everything green before installing gRPC.

After compiling from a clean main branch. Tests started failing and the dcs.log was filled with odd nil events from gRPC. Once i re applied this fix, tests are green again.

Do you have any recommendations about a proper methodology to ensure grpc errors do not break the dcs event handler?

rurounijones

See comment @ #232 (comment)

add guard agains nil group from dead unit

e5d9163

rkusa reviewed Jul 12, 2023

View reviewed changes

return nil for dead unit group

f147f40

rurounijones requested changes Jul 13, 2023

View reviewed changes

Merge branch 'DCS-gRPC:main' into main

a97d950

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add guard in lua against nil group from dead unit #232

add guard in lua against nil group from dead unit #232

uriba107 commented Jul 11, 2023 •

edited

Loading

rkusa left a comment

rkusa Jul 12, 2023

uriba107 Jul 12, 2023 •

edited

Loading

rurounijones Jul 13, 2023 •

edited

Loading

rkusa Jul 13, 2023 •

edited

Loading

rkusa Jul 13, 2023

rurounijones Jul 13, 2023 •

edited

Loading

uriba107 Jul 13, 2023

rurounijones Jul 16, 2023 •

edited

Loading

uriba107 Jul 17, 2023

uriba107 Sep 27, 2024

rurounijones left a comment

add guard in lua against nil group from dead unit #232

Are you sure you want to change the base?

add guard in lua against nil group from dead unit #232

Conversation

uriba107 commented Jul 11, 2023 • edited Loading

rkusa left a comment

Choose a reason for hiding this comment

rkusa Jul 12, 2023

Choose a reason for hiding this comment

uriba107 Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

rurounijones Jul 13, 2023 • edited Loading

Choose a reason for hiding this comment

rkusa Jul 13, 2023 • edited Loading

Choose a reason for hiding this comment

rkusa Jul 13, 2023

Choose a reason for hiding this comment

rurounijones Jul 13, 2023 • edited Loading

Choose a reason for hiding this comment

uriba107 Jul 13, 2023

Choose a reason for hiding this comment

rurounijones Jul 16, 2023 • edited Loading

Choose a reason for hiding this comment

uriba107 Jul 17, 2023

Choose a reason for hiding this comment

uriba107 Sep 27, 2024

Choose a reason for hiding this comment

rurounijones left a comment

Choose a reason for hiding this comment

uriba107 commented Jul 11, 2023 •

edited

Loading

uriba107 Jul 12, 2023 •

edited

Loading

rurounijones Jul 13, 2023 •

edited

Loading

rkusa Jul 13, 2023 •

edited

Loading

rurounijones Jul 13, 2023 •

edited

Loading

rurounijones Jul 16, 2023 •

edited

Loading