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(storage): avoid crashes after move #7045

Merged

Conversation

coryan
Copy link
Member

@coryan coryan commented Jul 22, 2021

The storage::Object*Stream classes should not crash when used after
moved-from. They do not guarantee what value they hold, but crashing on
functions that just query their state is unexpected. Note that we have
not defined what pre-conditions are required by each function, so we
could have decided to make it UB to call these functions. That seems too
hostile when "not crash but returns some undefined value" is less
hostile and about as easy to implement.

Fixes #7044


This change is Reviewable

The `storage::Object*Stream` classes should not crash when used after
moved-from. They do not guarantee what value they hold, but crashing on
functions that just query their state is unexpected. Note that we have
not defined what pre-conditions are required by each function, so we
could have decided to make it UB to call these functions. That seems too
hostile when "not crash but returns some undefined value" is less
hostile and about as easy to implement.
@product-auto-label product-auto-label bot added the api: storage Issues related to the Cloud Storage API. label Jul 22, 2021
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 22, 2021
@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: 3bff109b08c065eddae20f0503b9eb66c2f7718a

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

@codecov
Copy link

codecov bot commented Jul 22, 2021

Codecov Report

Merging #7045 (3bff109) into main (942c723) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #7045   +/-   ##
=======================================
  Coverage   94.47%   94.48%           
=======================================
  Files        1304     1304           
  Lines      112300   112319   +19     
=======================================
+ Hits       106100   106124   +24     
+ Misses       6200     6195    -5     
Impacted Files Coverage Δ
google/cloud/storage/object_read_stream.cc 75.00% <100.00%> (+8.33%) ⬆️
google/cloud/storage/object_stream_test.cc 100.00% <100.00%> (ø)
google/cloud/storage/object_write_stream.cc 98.07% <100.00%> (+8.94%) ⬆️
google/cloud/pubsub/subscriber_connection_test.cc 97.20% <0.00%> (-0.70%) ⬇️
...sub/internal/batching_publisher_connection_test.cc 97.60% <0.00%> (-0.21%) ⬇️
google/cloud/pubsub/samples/samples.cc 91.67% <0.00%> (-0.08%) ⬇️
...bigtable/examples/bigtable_hello_instance_admin.cc 83.51% <0.00%> (+2.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 942c723...3bff109. Read the comment docs.

@coryan coryan marked this pull request as ready for review July 22, 2021 22:12
@coryan coryan requested a review from a team as a code owner July 22, 2021 22:12
@coryan coryan merged commit 4f94bc1 into googleapis:main Jul 22, 2021
@coryan coryan deleted the fix-storage-avoid-crashes-after-move-from branch July 22, 2021 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the Cloud Storage API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

storage::ObjectWriteStream left in unusable state after Suspend() or moves
4 participants