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.

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

Fix an error when incomplete S3Client is destroyed #373

Merged
merged 2 commits into from
Nov 1, 2023
Merged

Fix an error when incomplete S3Client is destroyed #373

merged 2 commits into from
Nov 1, 2023

Conversation

bryanwweber
Copy link
Contributor

@bryanwweber bryanwweber commented Oct 30, 2023

If an error occurs during initialization of the S3Client instance, the initializer for the Client superclass is never called. This could happen, for example, when the S3Client has a bad profile name passed which botocore doesn't understand. The specific error is that the file_cache_mode attribute is not set because the superclass initializer is not called in the S3Client initializer if an error occurs during S3 session setup. This commit moves the superclass initializer to the top of the S3Client initializer to ensure that all superclass attributes are defined if/when any errors occur.

The same fix is applied for the Google client and the local client.

Closes #372


Contributor checklist:

  • I have read and understood CONTRIBUTING.md
  • Confirmed an issue exists for the PR, and the text Closes #issue appears in the PR summary (e.g., Closes #123).
  • Confirmed PR is rebased onto the latest base
  • Confirmed failure before change and success after change
  • Any generic new functionality is replicated across cloud providers if necessary
  • Tested manually against live server backend for at least one provider
  • Added tests for any new functionality
  • Linting passes locally
  • Tests pass locally
  • Updated HISTORY.md with the issue that is addressed and the PR you are submitting. If the top section is not `## UNRELEASED``, then you need to add a new section to the top of the document for your change.

@codecov
Copy link

codecov bot commented Oct 30, 2023

Codecov Report

Merging #373 (bd14bca) into master (ffbdd1c) will decrease coverage by 0.5%.
The diff coverage is 100.0%.

Additional details and impacted files
@@           Coverage Diff            @@
##           master    #373     +/-   ##
========================================
- Coverage    93.7%   93.2%   -0.5%     
========================================
  Files          22      22             
  Lines        1563    1563             
========================================
- Hits         1465    1458      -7     
- Misses         98     105      +7     
Files Coverage Δ
cloudpathlib/client.py 89.6% <100.0%> (ø)

... and 2 files with indirect coverage changes

@bryanwweber
Copy link
Contributor Author

I don't have a Windows machine available and all the tests pass locally for me on my Mac. I think I'll need some help to debug and fix that.

@pjbull
Copy link
Member

pjbull commented Oct 30, 2023

@bryanwweber I have a preference to do this with getattr in the __del__ method, which will be a little more resilient to future changes and not depend on order of execution.

E.g.,

    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()

@bryanwweber
Copy link
Contributor Author

🎉 great idea @pjbull! I'll make that change

If an error occurs during initialization of the S3Client instance, the
initializer for the Client superclass is never called. This could
happen, for example, when the S3Client has a bad profile name passed
which botocore doesn't understand. When the remainder of the class is
garbage collected (for example, at interpreter shutdown), Python calls
the __del__ method of the Client superclass. The specific error is that the
file_cache_mode attribute is not set because the superclass initializer
is not called in the S3Client initializer if an error occurs during S3
session setup.

This change uses getattr with a default value to avoid the
AttributeError in the __del__ method.
@bryanwweber
Copy link
Contributor Author

@pjbull Change is made, sorry for the delay!

@pjbull pjbull changed the base branch from master to 373-client-del-exception November 1, 2023 20:20
@pjbull
Copy link
Member

pjbull commented Nov 1, 2023

@bryanwweber I don't think the Windows failure is due to your change. Going to merge into a repo-local branch to see if that passes, and if not tweak the test on that branch.

@pjbull pjbull merged commit 954a7bd into drivendataorg:373-client-del-exception Nov 1, 2023
23 of 25 checks passed
@bryanwweber bryanwweber deleted the fix-del-bug-in-client branch November 1, 2023 20:49
pjbull added a commit that referenced this pull request Nov 1, 2023
* Fix an error when incomplete S3Client is destroyed

If an error occurs during initialization of the S3Client instance, the
initializer for the Client superclass is never called. This could
happen, for example, when the S3Client has a bad profile name passed
which botocore doesn't understand. When the remainder of the class is
garbage collected (for example, at interpreter shutdown), Python calls
the __del__ method of the Client superclass. The specific error is that the
file_cache_mode attribute is not set because the superclass initializer
is not called in the S3Client initializer if an error occurs during S3
session setup.

This change uses getattr with a default value to avoid the
AttributeError in the __del__ method.

* Update history file

Co-authored-by: Bryan Weber <[email protected]>
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.

AttributeError when __del__ is called on S3Client that errors during __init__
2 participants