Skip to content

Fixing a possible null pointer exception and removing unnecessary nullability for settings #99

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

Merged
MichaelGHSeg merged 2 commits into from
Apr 22, 2024
Merged

Fixing a possible null pointer exception and removing unnecessary nullability for settings #99

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a10d3be
Fixing a possible null pointer exception and removing unnecessary nul…
MichaelGHSeg Apr 22, 2024
eeefaa3
Merge branch 'main' of ssh://github.com/segmentio/Analytics-CSharp in…
MichaelGHSeg Apr 22, 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
20 changes: 5 additions & 15 deletions Analytics-CSharp/Segment/Analytics/Analytics.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,9 +176,9 @@ public virtual void Reset()
/// it's not recommended to be used in async method.
/// </summary>
/// <returns>Instance of <see cref="Settings"/></returns>
public virtual Settings? Settings()
public virtual Settings Settings()
{
Task<Settings?> task = SettingsAsync();
Task<Settings> task = SettingsAsync();
task.Wait();
return task.Result;
}
Expand All @@ -187,20 +187,10 @@ public virtual void Reset()
/// Retrieve the settings.
/// </summary>
/// <returns>Instance of <see cref="Settings"/></returns>
public virtual async Task<Settings?> SettingsAsync()
public virtual async Task<Settings> SettingsAsync()
{
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?
// System is a struct, so the default is an empty instance, not null
// might as well write /* if (true) */
if (system is System convertedSystem)
{
returnSettings = convertedSystem._settings;
}

return returnSettings;
System system = await Store.CurrentState<System>();
return system._settings;
}

#endregion
Expand Down
14 changes: 5 additions & 9 deletions Analytics-CSharp/Segment/Analytics/Timeline.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,26 +160,22 @@ internal void Add(Plugin plugin)
Analytics analytics = plugin.Analytics;
analytics.AnalyticsScope.Launch(analytics.AnalyticsDispatcher, async () =>
{
Settings? settings = await plugin.Analytics.SettingsAsync();

// This is basically never null, look at the comment in SettingsAsync
if (settings == null)
{
return;
}

Settings settings = await plugin.Analytics.SettingsAsync();

@dimitri-dev dimitri-dev Apr 23, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I know this is already merged but since you already fetch
System system = await analytics.Store.CurrentState<System>();
isn't it redundant to call SettingsAsync which calls
System system = await Store.CurrentState<System>();
return system._settings;

So you can only fetch system, and then on line 173 pass through system._settings.

I don't think it will be much of a performance difference, but it will remove an extra call to Store.CurrentState, which I assume is not cached?

@wenxi-zeng wenxi-zeng Apr 23, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it is cached in memory, but that's a good call out! the call to SettingsAsync is redundant. we can get ride of it in the next release. thanks for pointing this out!

@MichaelGHSeg MichaelGHSeg Apr 23, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I did consider it, but I like having the consolidated interface for settings retrieval. Then we have one place to change it in the future, etc. Or even just for debug breakpoints.

@dimitri-dev dimitri-dev Apr 23, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I mean, if it's cached then it's not a performance impact.
The other thing is - how likely do you think a change for fetching settings will occur?

Either way, I only see two calls to SettingsAsync, one of which is the call here and the other being a call that converts sync-to-async which is only mentioned in the Tests project.

There is no Settings-related logic here honestly, since you just fetch them "automagically" through the System struct.

// Fetch system afterwards for a minuscule but cool performance gain
System system = await analytics.Store.CurrentState<System>();

// Don't initialize unless we have updated settings from the web.
// CheckSettings will initialize everything added before then, so wait until other inits have happened.
// Check for nullability because CurrentState returns default(IState) which could make the .Count throw a NullReferenceException
if (system._initializedPlugins != null && system._initializedPlugins.Count > 0)
{
await analytics.Store.Dispatch<System.AddInitializedPluginAction, System>(new System.AddInitializedPluginAction(new HashSet<int>{plugin.GetHashCode()}));
plugin.Update(settings.Value, UpdateType.Initial);
plugin.Update(settings, UpdateType.Initial);
}
});
}


internal void Remove(Plugin plugin) => _plugins.RemoveAll(tempPlugin => tempPlugin == plugin);

internal RawEvent Execute(RawEvent incomingEvent)
Expand Down