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.

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

Guard a possibly null pathManager. #6179

Merged
merged 1 commit into from
Dec 14, 2023

Conversation

ericpromislow
Copy link
Contributor

If the 'settings-update' event is handled before settings are loaded at startup, pathManager could be null.

This happens when the first-run dialog happens before settings have been loaded.

Copy link
Contributor

@mook-as mook-as left a comment

Choose a reason for hiding this comment

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

Does that mean that when we get to the normal path (line 262/264) we make a new one, and potentially end up with two (possibly different) path managers in use at the same time?

If the 'settings-update' event is handled before settings are loaded, `pathManager`
could be null.

Also, extract initializing `pathManager` into a function.

- This happens from both changing a pref (any pref) in the
  first-run window, as well as during normal settings loading.

Note that from first-run we're calling `pathManager.enforce()`,
while from standard startup we're calling `integrationManager.enforce()`
If there's a problem here, it should be covered in a separate issue and PR.

Signed-off-by: Eric Promislow <epromislow@suse.com>
Comment on lines +300 to +302
if (pathManager) {
await pathManager.remove();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that the same as

Suggested change
if (pathManager) {
await pathManager.remove();
}
await pathManager?.remove();

@mook-as mook-as merged commit 3b09b9e into rancher-sandbox:main Dec 14, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants