Skip to content

Catch and ignore errors writing JSON files. #3288

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
May 2, 2017
Merged

Catch and ignore errors writing JSON files. #3288

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a12432d
Catch and ignore errors writing JSON files.
Apr 30, 2017
be30f8b
Bail early when writing the data file failes
Apr 30, 2017
743e012
Add a clarification to the docstring.
May 2, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 36 additions & 11 deletions mypy/build.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -839,6 +839,10 @@ def write_cache(id: str, path: str, tree: MypyFile,
old_interface_hash: str, manager: BuildManager) -> str:
"""Write cache files for a module.

Note that this mypy's behavior is still correct when any given
write_cache() call is replaced with a no-op, so error handling
code that bails without writing anything is okay.

Args:
id: module ID
path: module path
Expand Down Expand Up @@ -882,6 +886,7 @@ def write_cache(id: str, path: str, tree: MypyFile,
except OSError as err:
manager.log("Cannot get stat for {}: {}".format(path, err))
# Remove apparently-invalid cache files.
# (This is purely an optimization.)
for filename in [data_json, meta_json]:
try:
os.remove(filename)
Expand All @@ -897,12 +902,26 @@ def write_cache(id: str, path: str, tree: MypyFile,
data_mtime = os.path.getmtime(data_json)
manager.trace("Interface for {} is unchanged".format(id))
else:
with open(data_json_tmp, 'w') as f:
f.write(data_str)
f.write('\n')
data_mtime = os.path.getmtime(data_json_tmp)
os.replace(data_json_tmp, data_json)
manager.trace("Interface for {} has changed".format(id))
try:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested refactoring: Move the try statement to a separate utility function to make the flow of the enclosing function easier to follow. Currently the function is too long to fit entirely on my screen, which makes code reviews harder.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's unfortunate that the error handling (including comments) is longer than the main logic of the function, but I'm hesitant about breaking it up into smaller bits. Maybe it'll fit on your screen if I move the comments elsewhere (e.g. to the function's docstring)? Anyway, we can iterate on that after the merge.

with open(data_json_tmp, 'w') as f:
f.write(data_str)
f.write('\n')
os.replace(data_json_tmp, data_json)
data_mtime = os.path.getmtime(data_json)
except os.error as err:
# Most likely the error is the replace() call
# (see https://github.com/python/mypy/issues/3215).
manager.log("Error writing data JSON file {}".format(data_json_tmp))
# Let's continue without writing the meta file. Analysis:
# If the replace failed, we've changed nothing except left
# behind an extraneous temporary file; if the replace
# worked but the getmtime() call failed, the meta file
# will be considered invalid on the next run because the
# data_mtime field won't match the data file's mtime.
# Both have the effect of slowing down the next run a
# little bit due to an out-of-date cache file.
return interface_hash

mtime = st.st_mtime
size = st.st_size
Expand All @@ -922,12 +941,18 @@ def write_cache(id: str, path: str, tree: MypyFile,
}

# Write meta cache file
with open(meta_json_tmp, 'w') as f:
if manager.options.debug_cache:
json.dump(meta, f, indent=2, sort_keys=True)
else:
json.dump(meta, f)
os.replace(meta_json_tmp, meta_json)
try:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Like above, maybe move this to a separate function?

with open(meta_json_tmp, 'w') as f:
if manager.options.debug_cache:
json.dump(meta, f, indent=2, sort_keys=True)
else:
json.dump(meta, f)
os.replace(meta_json_tmp, meta_json)
except os.error as err:
# Most likely the error is the replace() call
# (see https://github.com/python/mypy/issues/3215).
# The next run will simply find the cache entry out of date.
manager.log("Error writing meta JSON file {}".format(meta_json_tmp))

return interface_hash

Expand Down