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

feat: Return ErrNotExist when a distro does not exist #106

Merged
merged 4 commits into from
Mar 13, 2024

Conversation

EduardGomezEscandell
Copy link
Collaborator

@EduardGomezEscandell EduardGomezEscandell commented Mar 12, 2024

I read somewhere (I think some python style guide) that there are two patterns to access resources:

  • Ask for permission: Check if the resource is available before using it.
  • Ask for forgiveness: Try to use the resource and emit a meaningful error if it is unavailable.

The first is inherently racy as the availability of the resource may change after asking for permission. This lead our implementation to become an ask for permission, then ask for forgiveness sandwich which is needlesly verbose:

// Ask for permission
if ok, err := distro.IsRegistered(); err != nil {
    return err
} else if !ok {
    return ...
}

if err := distro.DoTheThing(); err != nil {
    // Ask for forgiveness
    return err
}

This PR makes usage a bit safer and less redundant:

err := distro.DoTheThing()
if errors.Is(err, wsl.ErrNotExist) {
    // Ask for forgiveness
    return ...
} else if err != nil {
    return err
}

Note that internally we do ask for permission in some places, essentially every Win32 API call. This is because the error reporting of these DLL calls is atrocious, and we cannot produce a meaningful error message from them.


This is not for sport. The current implementation caused a test failure due to a resource availability race:

Failure: https://github.com/canonical/ubuntu-pro-for-wsl/actions/runs/8062559006/job/22022548242?pr=606#step:8:29

Code: https://github.com/canonical/ubuntu-pro-for-wsl/blob/f31559d796e8f2421acfbf4056e86e8f06854171/windows-agent/internal/distros/distro/properties.go#L34-L46


Part of UDENG-2385

@EduardGomezEscandell EduardGomezEscandell self-assigned this Mar 12, 2024
@EduardGomezEscandell EduardGomezEscandell marked this pull request as ready for review March 12, 2024 11:09
@EduardGomezEscandell EduardGomezEscandell requested a review from a team as a code owner March 12, 2024 11:09
Copy link
Collaborator

@CarlosNihelton CarlosNihelton left a comment

Choose a reason for hiding this comment

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

Makes sense to me.

distro_test.go Outdated Show resolved Hide resolved
@EduardGomezEscandell
Copy link
Collaborator Author

Force-pushed to rebase after #107. No new changes.

@EduardGomezEscandell EduardGomezEscandell merged commit bf12013 into main Mar 13, 2024
4 of 6 checks passed
@EduardGomezEscandell EduardGomezEscandell deleted the err-not-exist branch March 13, 2024 09:04
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