fix: cancel upload when BlobWriter exits with exception

[ddelange]

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]

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

[andrewsg]

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
  "

[ddelange]

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]

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.

[andrewsg]

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]

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]

@andrewsg how about this?

[andrewsg]

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

[ddelange]

@andrewsg ready for review again, now with unit tests

[andrewsg]

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]

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

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.

[ddelange]

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!

[ddelange]

@andrewsg can you run CI again?

[andrewsg]

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