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.

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

Jump to bottom

Add error logging and recovery #94

Merged
merged 13 commits into from
May 13, 2019
Merged

Add error logging and recovery #94

Changes from 10 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
baa3462
Add error logging and recovery
anayden May 7, 2019
2c88eac
Reformat
anayden May 7, 2019
9e8869b
Type annotations
anayden May 7, 2019
02a82f7
fix
anayden May 7, 2019
51da02b
Keep playing
anayden May 7, 2019
372c6b9
Fix lint
anayden May 7, 2019
f51a58e
more lint
anayden May 7, 2019
4af47c3
Add hack
anayden May 7, 2019
98745fa
Remove hack
anayden May 8, 2019
c4b4e97
Removed unused import
anayden May 8, 2019
020034f
Update debug logging
anayden May 13, 2019
3fafa21
fix
anayden May 13, 2019
ea85fae
Extend error logging
anayden May 13, 2019
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
18 changes: 16 additions & 2 deletions platform_storage_api/fs/local.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,11 @@
import logging
import os
import shutil
import stat
from concurrent.futures import ThreadPoolExecutor
from dataclasses import dataclass, replace
from pathlib import Path, PurePath
from typing import List, Optional
from typing import Any, List, Optional, Union

import aiofiles

Expand Down Expand Up @@ -203,10 +204,23 @@ async def get_filestatus(self, path: PurePath) -> FileStatus:
def _remove(self, path: PurePath) -> None:
concrete_path = Path(path)
if concrete_path.is_dir():
shutil.rmtree(concrete_path)
shutil.rmtree(concrete_path, onerror=self._handle_rmtree_error)
else:
concrete_path.unlink()

def _handle_rmtree_error(
self, func: Any, path: Union[str, os.PathLike], exc_info: Any
) -> Any:
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):
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

logger.warning("Access error")
# Try to change the permission of file
os.chmod(path, stat.S_IWUSR)
Copy link
Contributor

Choose a reason for hiding this comment

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

You may need to change the mode of the parent too. See python/cpython#10320.

# call the calling function again
func(path)
Copy link
Collaborator

Choose a reason for hiding this comment

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

won't this result in the call stack depletion? should we reraise the exception instead as @serhiy-storchaka has suggested?

Copy link
Contributor

Choose a reason for hiding this comment

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

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 .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

got it. thanks for the explanation guys.


async def remove(self, path: PurePath) -> None:
await self._loop.run_in_executor(self._executor, self._remove, path)

Expand Down