Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

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

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e5d9163
add guard agains nil group from dead unit
Jul 11, 2023
f147f40
return nil for dead unit group
Jul 12, 2023
a97d950
Merge branch 'DCS-gRPC:main' into main
uriba107 Sep 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lua/DCS-gRPC/exporters/object.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Wdyt?

Copy link
Author

@uriba107 uriba107 Jul 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@rurounijones rurounijones Jul 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
  • ???

Copy link
Collaborator

@rkusa rkusa Jul 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤪

Copy link
Contributor

@rurounijones rurounijones Jul 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@rurounijones rurounijones Jul 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. It will get rid of error messages
  2. 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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

return {
id = tonumber(group:getID()),
name = group:getName(),
Expand Down