Add error logging and recovery #94
Conversation
platform_storage_api/fs/local.py
Outdated
| logger.warning("Handling Error for file %s", path) | ||
| logger.warning(exc_info) | ||
| # Check if file access issue | ||
| if not os.access(path, os.W_OK): |
There was a problem hiding this comment.
The error handler can be called for different functions: scandir, open, stat, rmdir, unlink. In some cases the function is failed because the path is absent or is nor accessible (for example because the parent directory is absent or non-readable). os.access() will fail in tat case as well.
There was a problem hiding this comment.
I agree with you, but this code is only temporary needed to troubleshoot the issue with rmtree. I don't plan to use it as a permanent handler.
platform_storage_api/fs/local.py
Outdated
| if not os.access(path, os.W_OK): | ||
| logger.warning("Access error") | ||
| # Try to change the permission of file | ||
| os.chmod(path, stat.S_IWUSR) |
There was a problem hiding this comment.
You may need to change the mode of the parent too. See python/cpython#10320.
platform_storage_api/fs/local.py
Outdated
| # Try to change the permission of file | ||
| os.chmod(path, stat.S_IWUSR) | ||
| # call the calling function again | ||
| func(path) |
There was a problem hiding this comment.
The function may need other arguments (for example dir_fd). And calling it will not help if it is scandir or stat.
There was a problem hiding this comment.
Same as above: this is a temporary troubleshooting handler that is supposed to work with rmtree only.
|
See my attempt to fix the similar problem for TemporaryDirectory: python/cpython#10320. That PR has not been merged yet because I need to test it on Windows and analyze it for the case of possible infinite recursion. |
|
@serhiy-storchaka any ideas how to better handle this (4af47c3#diff-28f724c04d913ea0e52fd0278f9b4f8fR211)? My solutions looks ugly and perhaps I'm missing something. |
|
The type of the second parameter is |
|
You can use |
I do, but I somehow missed the last issue. My bad. |
|
@serhiy-storchaka does the code look reasonably good to you? I only want to get more debugging output here for further fixes. |
|
If you want to just log the error, it is enough to log it and reraise the exception. Calling the failed function again not always has effect. But I think this will not make the situation worse. |
platform_storage_api/fs/local.py
Outdated
| # Try to change the permission of file | ||
| os.chmod(path, stat.S_IWUSR) | ||
| # call the calling function again | ||
| func(path) |
There was a problem hiding this comment.
won't this result in the call stack depletion? should we reraise the exception instead as @serhiy-storchaka has suggested?
There was a problem hiding this comment.
On other hand, if just log and reraise the error -- this does not differ from catching and logging error outside of the rmtree().
But if the goal is fixing the error, calling the function again is not enough. You will need more complex solution like in python/cpython#10320 .
There was a problem hiding this comment.
My understanding was that the call on the line 222 would have onerror=None and hence raise the exception on error. Isn't it the case?
There was a problem hiding this comment.
@dalazx Answering your question, there is no recursion. func is not recursive and the onerror handler is not called for the exception raised in the onerror handler.
But we can get a leak of the file descriptor if func is os.open and the repeated call is finished successfully for some reasons.
There was a problem hiding this comment.
got it. thanks for the explanation guys.
| # Debug logging | ||
| path = e.filename | ||
| parent_path = os.path.dirname(path) | ||
| path_access_ok = os.access(path, os.W_OK) |
There was a problem hiding this comment.
It would be nice to output a mode.
try:
path_mode = f"{os.stat(path).st_mode:03o}"
except OSError:
path_mode = '?'
platform_storage_api/fs/local.py
Outdated
| path_access_ok = os.access(path, os.W_OK) | ||
| parent_path_access_ok = os.access(parent_path, os.W_OK) | ||
| logger.warning( | ||
| f"OSError for path %s, access = %s parent_access = %s", |
There was a problem hiding this comment.
Add also the error message.
Either use the f-string for formatting the whole message or do not use the f prefix.
There was a problem hiding this comment.
Do you mean e.strerror?
There was a problem hiding this comment.
And the errno attribute. Or better errno.errorcode.get(err.errno, err.errno).
There was a problem hiding this comment.
Do you mean
e.strerror?
str(e)
This is to investigate the issue in neuro-inc/apolo-cli#752