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

Update makedirs function to not supress OSErrors #3404

Open
wants to merge 6 commits into
base: proton_4.11
Choose a base branch
from

Conversation

SkyLeite
Copy link

@SkyLeite SkyLeite commented Jan 3, 2020

The current implementation of makedirs suppresses every OSError possible and assumes the directory already exists when one is raised. This is problematic as there are many reasons why the error could occur, and suppressing it could make it hard to trace the problem back in the future.

My implementation fixes this by checking whether the path exists before attempting to create it, and logs every other OSError appropriately.

I also took the liberty of changing a stray os.makedirs call to our makedirs function.

@aeikum
Copy link
Collaborator

aeikum commented Jan 3, 2020

Looks reasonable, thanks. Will merge.

@SkyLeite
Copy link
Author

SkyLeite commented Jan 4, 2020

Updated a different occurrence of this as well. We're also ignoring errors in the set_dir_casefold_bit but I don't know what that function is supposed to do so I'm not touching it for now.

@aeikum
Copy link
Collaborator

aeikum commented Jan 6, 2020

The logic seems fine, but is listdir and len the best way to determine if a directory is empty? This code isn't terribly perf-critical, but maybe doing something with a scandir iterator would be more efficient? (I'm not a Python expert; maybe the optimizer is smarter than I think?)

@SkyLeite
Copy link
Author

SkyLeite commented Jan 6, 2020

From what I could gather from the scandir proposal, that seems to be correct. The latest commit should address that.

proton Outdated
os.makedirs(path)
except OSError:
#already exists
except OSError as err:
Copy link
Contributor

Choose a reason for hiding this comment

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

Use FileExistsError instead of OSError

proton Outdated
#not empty
if not any(os.scandir(d)):
os.rmdir(d)
except OSError as err:
Copy link
Contributor

@nanonyme nanonyme Jan 10, 2020

Choose a reason for hiding this comment

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

Instead

except OSError as e:
    if e.errno != errno.ENOTEMPTY:
      raise

Copy link
Author

Choose a reason for hiding this comment

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

As far as I could gather checking if the directory is empty before attempting to do the operation is less expensive than letting it throw an exception and catching it. Would we still prefer going with the latter approach?

Copy link
Contributor

@nanonyme nanonyme Jan 13, 2020

Choose a reason for hiding this comment

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

There's no measurable performance difference between the two (although the deletion and seeing whether it fails is most likely faster as it results in less syscalls on average) but with the exception handling race conditions are impossible. That's typically recommended way to handle these things.

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense. I hadn't considered race conditions. I'll make the changes in a bit :)

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.

3 participants