-
Notifications
You must be signed in to change notification settings - Fork 19
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
DM-44462: Try to trap case where another eups process deletes the db file #152
Conversation
No need to use a regex and store the value since it's used twice.
Allows it to easily be used in a with block.
Some other EUPS process could remove the db file that was created by this process.
fd = open(fileName, "rb") | ||
lookup = pickle.load(fd) | ||
fd.close() | ||
with open(fileName, "rb") as fd: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is littered with the old style opens. I have only fixed the one that was in the same file as I was editing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what I'm supposed to do about the race in this code. Maybe it's a bit early in the day for me but that code gets the file name from the flavor, calls stat without checking if the cache file is there, then tries to open it. I think the code is assuming that the reload
call is gated by a check higher up.
if file not in self.modtimes: | ||
return True | ||
try: | ||
older = os.stat(file).st_mtime <= self.modtimes[file] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seem to be a number of similar os.stat()
calls. They don't need protection as well?
It's curious that the only thing that seems to remove this file is an admin command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will take a look at the other places.
I was also wondering how the file disappears even in the case where 30 EUPS processes in parallel are trying to read the same database file. The atomic rename should not result in the file disappearing so I realize that this is papering over the cracks a little. @RobertLuptonTheGood do you know of a way that the pickle file could be deleted in normal usage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a couple more traps but reload
is a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if this isn't the ultimate solution, it still looks better than what was there before.
No description provided.