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

closed is True right after opening a S3File #1310

Closed
ahumeau opened this issue Sep 28, 2023 · 2 comments · Fixed by #1311
Closed

closed is True right after opening a S3File #1310

ahumeau opened this issue Sep 28, 2023 · 2 comments · Fixed by #1311

Comments

@ahumeau
Copy link

ahumeau commented Sep 28, 2023

Hey there!

Description

After updating to 1.14 I encountered the following issue:

storage = S3Storage(bucket_name="my-bucket")
file = storage.open("my-file")
file.closed
> True  # Should be False

Analysis

I pinpointed the issue to commit af91e2c.

The issue it fixes is perfectly legitimate but the way it fixes it introduces this bug.

S3File implements some kind of lazy opening by leveraging the file property (and its _get_file accessor). The file is not actually opened until the file property is first accessed, self._file remains None until then.
With the current closed implementation, self._file being None results in closed returning False even though in this case the file was never closed, it was never even read from.

Proposed fixes

I see two approaches:

  • keeping track of whether a S3File is opened of closed through a _closed attribute (for example), setting it to True in S3File.__init__, to False in S3File.close and reading it in S3File.closed
  • forcing _file to be defined by calling f.file in S3Storage._open

I have no idea which is better (might be another approach altogether).

NB: might be related to #1308

@ahumeau ahumeau changed the title S3File closed right after opening it closed is True right after opening a S3File Sep 28, 2023
@ahumeau ahumeau changed the title closed is True right after opening a S3File closed is True right after opening a S3File Sep 28, 2023
@jschneier
Copy link
Owner

Thanks for the comprehensive report, will look into this

@ahumeau
Copy link
Author

ahumeau commented Sep 29, 2023

Thanks @jschneier!

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 a pull request may close this issue.

2 participants