Skip to content

Don't use os.replace #3221

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

Closed
wants to merge 1 commit into from
Closed

Don't use os.replace #3221

wants to merge 1 commit into from

Conversation

pkch
Copy link
Contributor

@pkch pkch commented Apr 22, 2017

Fix #3215

For now, os.remove is still left in the code. Let's see if the problem is solved by just writing directly to the file instead of writing to a temp file and deleting the original. I don't recall problems with Windows-specific race conditions around open ('filename', 'w'); only rename and delete are affected.

@refi64
Copy link
Contributor

refi64 commented Apr 22, 2017

I thought the whole point of the temp file was to avoid ending up with a corrupted cache if someone Ctrl-C'd mypy?

@pkch
Copy link
Contributor Author

pkch commented Apr 22, 2017

When that os.replace is invoked, we already know the cache file is stale. I thought mypy treats stale and corrupt cache identically - but I was wrong, in case of corrupt cache, mypy might actually crash (no try/except around json.load).

So I agree with you, but perhaps a better solution is to handle corrupt cache files gracefully? While unlikely, corruption could occur even without my PR (some unrelated program can accidentally overwrite mypy cache files with garbage). More importantly, fixing os.replace seems to be far more complex than handling corrupt cache.

@gvanrossum
Copy link
Member

I wrote that code being very careful of not causing race conditions due to the process being killed hard between any two. But I also intentionally left all I/O error checking to future generations.

@pkch
Copy link
Contributor Author

pkch commented Apr 22, 2017

More importantly, fixing os.replace seems to be far more complex than handling corrupt cache.

I take it back... it's not that easy to deal with corrupt cache. Well, it's easy with meta files. But the data files (which of course we don't want to read twice) are read at a very late stage. By then if it turns out the data file is corrupt, there's nothing simple we can do (it's not like we can just return None or create an empty MypyFile at that point).

If you think it's ok to just delete the entire cache and restart the incremental check, I can do that (if cache corruption is very rare, I guess it's ok?).

Alternatively, we can give up on checking for corruption, and go back to finding workarounds for os.replace.

@refi64
Copy link
Contributor

refi64 commented Apr 22, 2017

If I've learned anything from working with IO issues on Windows, it's that the best way to "fix" them is to keep trying until either:

  • it works
  • the user's CPU burns, the fire spreads to a nuclear missile switch, and the world ends

@pkch
Copy link
Contributor Author

pkch commented Apr 22, 2017

@kirbyfan64 Seems like it..

Given that all we want to do is an atomic file write (it even has nothing to do with python), I thought there's gotta be a way to do it on Windows without the file delete / rename race condition. So I searched for a while, but concluded that really, there isn't.

@gvanrossum
Copy link
Member

See more discussion in #3215. Maybe this PR is dead?

@pkch
Copy link
Contributor Author

pkch commented Apr 22, 2017

Yup :)

@pkch pkch closed this Apr 22, 2017
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