Skip to content

Commit

Permalink
weakref finalize instead of __del__
Browse files Browse the repository at this point in the history
  • Loading branch information
pjbull committed Aug 22, 2024
1 parent 6fc3a1e commit e33b583
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 28 deletions.
10 changes: 10 additions & 0 deletions cloudpathlib/cache_utils.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
from pathlib import Path
import shutil


def _clear_cache(_local: Path) -> None:
if _local.exists():
if _local.is_file():
_local.unlink()
else:
shutil.rmtree(_local)
27 changes: 15 additions & 12 deletions cloudpathlib/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
import shutil
from tempfile import TemporaryDirectory
from typing import Generic, Callable, Iterable, Optional, Tuple, TypeVar, Union
import weakref

from .cache_utils import _clear_cache
from .cloudpath import CloudImplementation, CloudPath, implementation_registry
from .enums import FileCacheMode
from .exceptions import InvalidConfigurationException
Expand All @@ -26,6 +28,18 @@ def decorator(cls: type) -> type:
return decorator


def _client_finalizer(file_cache_mode: FileCacheMode, _local_cache_dir) -> None:
if file_cache_mode in [
FileCacheMode.tmp_dir,
FileCacheMode.close_file,
FileCacheMode.cloudpath_object,
]:
_clear_cache(_local_cache_dir)

if _local_cache_dir.exists():
_local_cache_dir.rmdir()

Check warning on line 40 in cloudpathlib/client.py

View check run for this annotation

Codecov / codecov/patch

cloudpathlib/client.py#L40

Added line #L40 was not covered by tests


class Client(abc.ABC, Generic[BoundedCloudPath]):
_cloud_meta: CloudImplementation
_default_client = None
Expand Down Expand Up @@ -82,18 +96,7 @@ def __init__(

self.file_cache_mode = file_cache_mode

def __del__(self) -> None:
# remove containing dir, even if a more aggressive strategy
# removed the actual files
if getattr(self, "file_cache_mode", None) in [
FileCacheMode.tmp_dir,
FileCacheMode.close_file,
FileCacheMode.cloudpath_object,
]:
self.clear_cache()

if self._local_cache_dir.exists():
self._local_cache_dir.rmdir()
weakref.finalize(self, _client_finalizer, self.file_cache_mode, self._local_cache_dir)

@classmethod
def get_default_client(cls) -> "Client":
Expand Down
36 changes: 20 additions & 16 deletions cloudpathlib/cloudpath.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
_PathParents,
)

import shutil
import sys
from typing import (
overload,
Expand All @@ -33,6 +32,7 @@
)
from urllib.parse import urlparse
from warnings import warn
import weakref

if sys.version_info >= (3, 10):
from typing import TypeGuard
Expand All @@ -58,6 +58,7 @@ def _make_selector(pattern_parts, _flavour, case_sensitive=True):

from . import anypath

from .cache_utils import _clear_cache
from .exceptions import (
ClientMismatchError,
CloudPathFileExistsError,
Expand Down Expand Up @@ -188,6 +189,20 @@ def __init__(cls, name: str, bases: Tuple[type, ...], dic: Dict[str, Any]) -> No
getattr(cls, attr).fget.__doc__ = docstring


def _cloudpath_finalizer(handle: Optional[IO], client: Optional["Client"], _no_prefix: str):
"""Use weakref.finalizer instead of __del__ since it is more reliable (does
not wait for garbage collection to actually run).
"""
# make sure that file handle to local path is closed
if handle is not None:
handle.close()

Check warning on line 198 in cloudpathlib/cloudpath.py

View check run for this annotation

Codecov / codecov/patch

cloudpathlib/cloudpath.py#L198

Added line #L198 was not covered by tests

# ensure file removed from cache when cloudpath object deleted
if client is not None:
if getattr(client, "file_cache_mode", None) == FileCacheMode.cloudpath_object:
_clear_cache(client._local_cache_dir / _no_prefix)


# Abstract base class
class CloudPath(metaclass=CloudPathMeta):
"""Base class for cloud storage file URIs, in the style of the Python standard library's
Expand Down Expand Up @@ -242,23 +257,16 @@ def __init__(
# track if local has been written to, if so it may need to be uploaded
self._dirty = False

# register cache cleanup method when this object is marked for garbage collection
weakref.finalize(self, _cloudpath_finalizer, self._handle, self._client, self._no_prefix)

@property
def client(self):
if getattr(self, "_client", None) is None:
self._client = self._cloud_meta.client_class.get_default_client()

return self._client

def __del__(self) -> None:
# make sure that file handle to local path is closed
if self._handle is not None:
self._handle.close()

# ensure file removed from cache when cloudpath object deleted
client = getattr(self, "_client", None)
if getattr(client, "file_cache_mode", None) == FileCacheMode.cloudpath_object:
self.clear_cache()

def __getstate__(self) -> Dict[str, Any]:
state = self.__dict__.copy()

Expand Down Expand Up @@ -1088,11 +1096,7 @@ def copytree(self, destination, force_overwrite_to_cloud=None, ignore=None):

def clear_cache(self):
"""Removes cache if it exists"""
if self._local.exists():
if self._local.is_file():
self._local.unlink()
else:
shutil.rmtree(self._local)
_clear_cache(self._local)

# =========== private cloud methods ===============
@property
Expand Down

0 comments on commit e33b583

Please sign in to comment.