FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix: cancel upload when BlobWriter exits with exception by ddelange · Pull Request #1243 · googleapis/python-storage · GitHub

This repository was archived by the owner on Mar 31, 2026. It is now read-only.
/ python-storage Public archive

fix: cancel upload when BlobWriter exits with exception - #1243

Merged
andrewsg merged 1 commit into
googleapis:3.0-develfrom
ddelange:patch-2
Jul 31, 2024
Merged

fix: cancel upload when BlobWriter exits with exception#1243
andrewsg merged 1 commit into
googleapis:3.0-develfrom
ddelange:patch-2

Conversation

ddelange commented Mar 22, 2024
edited
Loading

Copy link
Copy Markdown
Contributor

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #1228

ddelange requested review from a team March 22, 2024 06:24
product-auto-label Bot added size: s Pull request size is small. api: storage Issues related to the googleapis/python-storage API. labels Mar 22, 2024
ddelange force-pushed the patch-2 branch 3 times, most recently from 7324818 to 539120b Compare March 22, 2024 06:54
product-auto-label Bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Mar 22, 2024

Copy link
Copy Markdown
Contributor Author

@andrewsg can you link me to the relevant documentation section(s) to update? do you have specific wording in mind?

andrewsg changed the base branch from main to 3.0-devel July 11, 2024 18:09
andrewsg added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 11, 2024
yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 11, 2024

andrewsg commented Jul 11, 2024
edited
Loading

Copy link
Copy Markdown
Contributor

Hi @ddelange,

Thanks for your submission. I've retargeted the PR to the 3.0 branch. For safety the test runner has to be activated manually for external PRs, so I've started the tests running now.

Three things:

  • Please add the following text to the README file, at the end of the "Major Version Release Notes" section, right above the "Quick Start":
Miscellaneous
~~~~~~~~~~~~~

- The BlobWriter class now attempts to terminate an ongoing resumable upload if
  the writer exits with an exception.
  • Somehow a typo in the new docs file escaped the CI last round. Could you please add a single ~ character to the title underline in the file docs/storage/exceptions.rst, to clear the "title underline isn't long enough" warning? It isn't related to this PR, but it's too trivial to make a separate PR for if we can clean it up here.

  • The system test has this error:

        # no-op when nothing was uploaded yet
        with pytest.raises(ValueError, match="SIGTERM received"):
            with blob.open("wt") as writer:
>               writer.write(b'first chunk')  # not yet uploaded
E               TypeError: write() argument must be str, not bytes
  • I don't see it yet but there will probably be a coverage issue, as features have to be exercised by unit tests. Please add a unit test.
    Edit: Coverage says "google/cloud/storage/fileio.py 204 7 54 0 96% 442-445, 448-451
    "

Copy link
Copy Markdown
Contributor Author

Hi @andrewsg 👋

I rebased my commit to the new target branch, and fixed points 1 2 and 3. Point 3 was my typo in the tests, causing them to exit early. With the test passing, point 4 should also be covered (literally :]).

andrewsg added the kokoro:run Add this label to force Kokoro to re-run the tests. label Jul 12, 2024
yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Jul 12, 2024

Copy link
Copy Markdown
Contributor

Thank you for doing all of that! Unfortunately, system tests exist alongside the unit tests but they aren't a replacement for unit tests in the coverage requirements for this package, so we still will need unit tests covering the lines mentioned for the PR to pass.

Copy link
Copy Markdown
Contributor

Looks like there's another system test issue:

shared_bucket = , blobs_to_delete = []
service_account = 

    def test_blobwriter_exit(
        shared_bucket,
        blobs_to_delete,
        service_account,
    ):
        blob = shared_bucket.blob("NeverUploaded")
    
        # no-op when nothing was uploaded yet
        with pytest.raises(ValueError, match="SIGTERM received"):
            with blob.open("wb") as writer:
                writer.write(b"first chunk")  # not yet uploaded
                raise ValueError("SIGTERM received")  # no upload to cancel in __exit__
        # blob should not exist
        assert not blob.exists()
    
        # unhandled exceptions should cancel the upload
        with pytest.raises(ValueError, match="SIGTERM received"):
            with blob.open("wb") as writer:
                writer.write(b"first chunk")  # not yet uploaded
>               writer.write(b"big chunk" * 1024 ** 8)  # uploaded
E               OverflowError: cannot fit 'int' into an index-sized integer

Let me know if you need help with running the system tests locally.

ddelange commented Jul 13, 2024
edited
Loading

Copy link
Copy Markdown
Contributor Author

Thanks for the heads up. I'll write a new unit test case. Regarding the OverflowError: can you suggest a cleaner way to trigger the threshold where the first part is uploaded (multipart upload created)? Is there a certain amount of bytes stored in a constant I can import? To change the line to something like writer.write(b'0' * MIN_PART_UPLOAD_SIZE)?

ddelange left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@andrewsg how about this?

Comment thread tests/system/test_fileio.py Outdated
Comment thread tests/system/test_fileio.py Outdated

Copy link
Copy Markdown
Contributor

Hi ddlange, yes, overriding the chunk size makes sense to me. Go for it.

ddelange changed the title Cancel upload when BlobWriter exits with exception fix: cancel upload when BlobWriter exits with exception Jul 19, 2024

Copy link
Copy Markdown
Contributor Author

@andrewsg ready for review again, now with unit tests

andrewsg added the kokoro:run Add this label to force Kokoro to re-run the tests. label Jul 19, 2024
yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Jul 19, 2024

Copy link
Copy Markdown
Contributor

Thank you. Looks like there are still test failures with the system test and with the unit test. Can you take a look? Run "nox -s unit-3.11" (or whatever version) to run the unit tests, subsequently "nox -s cover" to check coverage, and "nox -s blacken lint" to get lint to pass as well.

ddelange force-pushed the patch-2 branch 2 times, most recently from 59b5db3 to 7645921 Compare July 22, 2024 15:04

Copy link
Copy Markdown
Contributor Author

@andrewsg thanks for the heads up.

  • fixed system test typo chunksize -> chunk_size
  • fixed unit test typo initiate_resumable_upload -> _initiate_resumable_upload in my and multiple other unit tests in test_fileio.py (I guess it was a typo which propagated through copy-paste)

unit tests are green on my machine, but nox -s cover still shows 442-445, 448-451 missing, which are the complete functions this PR adds. how can that be? the unit tests I wrote should cover all lines I added 🤔

andrewsg added the kokoro:run Add this label to force Kokoro to re-run the tests. label Jul 23, 2024
yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Jul 23, 2024

Copy link
Copy Markdown
Contributor

Thank you for catching the unit test issue in the other tests. It would have been avoided if I'd used the "spec" features of the mocking library properly; my mistake. I appreciate you fixing it in the entire file.

I don't know why coverage doesn't work on your machine - it's finicky - but it is clear in CI so all is good with the unit tests and coverage.

The system test is still failing. The part of the test that was causing an error previously due to expanding too aggressively, which you fixed by using the chunk size as the size to write, is repeated further down in the test.

Copy link
Copy Markdown
Contributor Author

which you fixed by using the chunk size as the size to write, is repeated further down in the test

thanks for the catch, fixed now!

Copy link
Copy Markdown
Contributor Author

@andrewsg can you run CI again?

andrewsg added the kokoro:run Add this label to force Kokoro to re-run the tests. label Jul 30, 2024
yoshi-kokoro removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Jul 30, 2024

Copy link
Copy Markdown
Contributor

@ddelange Checks pass. Thank you for your contribution, and again thanks for the heads-up about the testing issue.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

api: storage Issues related to the googleapis/python-storage API. size: m Pull request size is medium.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BlobWriter should abort multipart upload during exception handling

4 participants


Back | FazBrowse Home | New Git URL