Introduce unified checksum options and deprecate old options#2
Introduce unified checksum options and deprecate old options#2v-pratap wants to merge 9 commits into
Conversation
…oads finalize (googleapis#16245) * fix(storage): allow providing expected CRC32C to Finalize() for appendable uploads * fix(storage): address reviewer comments on checksum logic * fix(storage): correctly merge options and handle MD5 for appendable uploads * refactor(storage): clarify Finalize() checksum logic to avoid apparent redundancy
There was a problem hiding this comment.
Code Review
This pull request introduces new options for configuring upload and download checksum validation (UploadChecksumValidationOption and DownloadChecksumValidationOption) while deprecating older hashing options. It also updates the asynchronous writer connection to support manual checksum verification during finalization and adds corresponding tests and examples. The review feedback highlights two critical issues: first, the unsafe use of OptionsSpan across a co_await suspension point in the new sample code, which can leak thread-local state; second, a bug in AsyncWriterConnectionImpl::Finalize where current_options is checked instead of merged for non-appendable uploads, causing initial upload options to be ignored.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // Set the expected CRC32C checksum in the current options scope | ||
| // just before calling Finalize(). | ||
| // Note: 548262564U is the pre-computed CRC32C checksum for the string "Some | ||
| // data\n". If the data changes, this checksum must be updated to match. | ||
| google::cloud::internal::OptionsSpan span( | ||
| google::cloud::Options{}.set<gcs::UseCrc32cValueOption>(548262564U)); | ||
| co_return (co_await writer.Finalize(std::move(token))).value(); |
There was a problem hiding this comment.
Using google::cloud::internal::OptionsSpan across a co_await point is highly dangerous. OptionsSpan is an RAII object that manages thread-local state. Because a coroutine can suspend at co_await and resume on a different thread (or allow other tasks to run on the same thread in the meantime), keeping OptionsSpan alive across co_await can leak the options context to unrelated tasks or fail to restore the thread-local state correctly on resumption.
To fix this, invoke writer.Finalize synchronously inside a lambda or block where the OptionsSpan is created and destroyed before the co_await suspension point.
// Set the expected CRC32C checksum in the current options scope
// just before calling Finalize().
// Note: 548262564U is the pre-computed CRC32C checksum for the string "Some
// data\n". If the data changes, this checksum must be updated to match.
auto finalize_future = [&] {
google::cloud::internal::OptionsSpan span(
google::cloud::Options{}.set<gcs::UseCrc32cValueOption>(548262564U));
return writer.Finalize(std::move(token));
}();
co_return (co_await std::move(finalize_future)).value();| } else if (current_options | ||
| .has<google::cloud::storage::UseCrc32cValueOption>() || | ||
| current_options.has<google::cloud::storage::UseMD5ValueOption>()) { | ||
| // If the user specified a manual expected checksum dynamically at | ||
| // Finalize() via current_options, we manually inject it and use `kFinalize` | ||
| // so the internal hash function doesn't overwrite it with its own computed | ||
| // hash. | ||
| if (current_options.has<google::cloud::storage::UseCrc32cValueOption>()) { | ||
| write.mutable_object_checksums()->set_crc32c( | ||
| current_options.get<google::cloud::storage::UseCrc32cValueOption>()); | ||
| } | ||
| if (current_options.has<google::cloud::storage::UseMD5ValueOption>()) { | ||
| write.mutable_object_checksums()->set_md5_hash( | ||
| current_options.get<google::cloud::storage::UseMD5ValueOption>()); | ||
| } | ||
| action = PartialUpload::kFinalize; | ||
| } |
There was a problem hiding this comment.
In the else if block for non-appendable uploads, the code checks current_options instead of merged. This means that if the user specified UseCrc32cValueOption or UseMD5ValueOption at the start of the upload (which is stored in options_ and merged into merged), those options will be completely ignored during Finalize().
To ensure options specified at both the start of the upload and dynamically at Finalize() are respected, use merged instead of current_options in this block, consistent with how appendable uploads are handled.
} else if (merged.has<google::cloud::storage::UseCrc32cValueOption>() ||
merged.has<google::cloud::storage::UseMD5ValueOption>()) {
// If the user specified a manual expected checksum, we manually inject it
// and use `kFinalize` so the internal hash function doesn't overwrite it
// with its own computed hash.
if (merged.has<google::cloud::storage::UseCrc32cValueOption>()) {
write.mutable_object_checksums()->set_crc32c(
merged.get<google::cloud::storage::UseCrc32cValueOption>());
}
if (merged.has<google::cloud::storage::UseMD5ValueOption>()) {
write.mutable_object_checksums()->set_md5_hash(
merged.get<google::cloud::storage::UseMD5ValueOption>());
}
action = PartialUpload::kFinalize;
}1264fa0 to
55ceb4c
Compare
55ceb4c to
6580fd9
Compare
6580fd9 to
6c92722
Compare
Introduces unified enum-based
UploadChecksumValidationOptionandDownloadChecksumValidationOptionand deprecates the old bool-basedEnableMD5ValidationOptionandEnableCrc32cValidationOption.