NullReferenceException when fetching System state #98
Conversation
|
Mind taking a look at this fix @wenxi-zeng ? |
|
Have you got a moment to check this out? @MichaelGHSeg |
dimitri-dev
changed the title
|
Sorry for the delay in responding. I've looked over the code and I'm hesitant to declare it 'approved'. I think that it will behave as expected, but there are a few other changes that need to be made for me to be confident that it won't fail quietly. I also want Wenxi's input and he should be back soon. |
|
@MichaelGHSeg I have only taken a look at the bits surrounding the issue/exception that occured. If I can be of any more help, either by testing or by coding, let me know! |
| Settings? returnSettings = null; | ||
| IState system = await Store.CurrentState<System>(); | ||
|
|
||
| // I don't understand this as Store.CurrentState will at worst return a default(TState) which will be a default(System) in this case, hence this cast will always go through? |
There was a problem hiding this comment.
that's a good question. this check here only serves for a conversion purpose that converts an IState to System. the check alone is unnecessary.
There was a problem hiding this comment.
But you can also just accept var system = await Store.CurrentState<System>() or even System system = await Store.CurrentState<System>() and it would be fine?
I mean, CurrentState accepts TState generic and also returns a Task, so whatever you call it with, it should return as?
There was a problem hiding this comment.
yeah. we were doing it with var but had to change it to the current way for compatibility reason. I believe accept it as System should just be fine. no idea why we didn't do it 🤦
There was a problem hiding this comment.
I mean, coding style aside - the real question is why the conversion check exists, and why does this method return Settings? when the CurrentState method returns default(TState) or the TState instance, in this case System.
And the System struct holds a Settings struct, which cannot be null again.
Is the property in System supposed to be Settings? (a nullable struct) maybe?
There was a problem hiding this comment.
oh. I see what you're saying. the design of the system properties are on purpose. it's the way we chose to write the code made that SettingsAsync to return Settings?. the function will be updated accordingly with the removal of the type check.
| Settings? settings = await plugin.Analytics.SettingsAsync(); | ||
|
|
||
| // This is basically never null, look at the comment in SettingsAsync | ||
| if (settings == null) |
There was a problem hiding this comment.
that's a good performance optimization!
There was a problem hiding this comment.
But it's not really a performance optimization if you are never returning a null object. :/
| if (settings.HasValue && system._initializedPlugins.Count > 0) | ||
|
|
||
| // Check for nullability because CurrentState returns default(IState) which could make the .Count throw a NullReferenceException | ||
| if (system._initializedPlugins != null && system._initializedPlugins.Count > 0) |
There was a problem hiding this comment.
I'm surprised _initializedPlugins could be null, since we have a null check in the constructor that converts it to be an empty set if it's null. but from my previous investigation this line is indeed the line causing the NRE. it might be the the environment.
There was a problem hiding this comment.
Not quite. You have a constructor with 4 parameters, and an optionable HashSet parameter. When returning a default(struct), it targets the default constructor. To my understanding, since the parameters are not available when creating a default instance, it cannot call it - hence you get an "empty" object.
In this case, a solution also could be to just add a default constructor like so:
internal System() { _initializedPlugins = new(); }
And it would avoid this check.
|
Thanks! |
Uhhh, will probably remove the comments. But there are some valid concerns in the code.
My main question is: do you realize that returning a default of a struct is not a null value, but rather an empty instance?
TLDR: NullReferenceException in #97 is caused by an uninitialized HashSet.