fix(storage): sign_with is Send#4399
Conversation
… `async fn sign_with` is send Update signed_url.rs to ensure that when canonical_query is created (using a Serializer which is non-send), we drop the Serializer at that point so it doesn't remain alive until the end of the scope. Currently even though the canonical_query variable is shadowed, rust does not automatically drop the shadowed variable until the end of scope, and `.finish()` only takes an `&mut Serializer`. So the original non-send `canonical_query` lives for the whole scope across an await point and therefore makes `sign_with` non-send.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
/gcbrun |
sign_with is sendsign_with is send
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4399 +/- ##
==========================================
+ Coverage 94.93% 94.94% +0.01%
==========================================
Files 191 191
Lines 7262 7261 -1
==========================================
Hits 6894 6894
+ Misses 368 367 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
sign_with is sendsign_with is Send
dbolduc
left a comment
There was a problem hiding this comment.
Hey, you are totally right. Good catch, and thanks for the fix!
Our linter is unhappy with the formatting though. We need to fix that before merging.
Also, optionally, can you add a unit test so that this doesn't happen again? I was thinking something like the following (which I tested locally):
fn assert_send<T: Send>(_t: &T) {}
#[tokio::test]
async fn sign_with_is_send() -> TestResult {
let mut mock = MockSigner::new();
mock.expect_client_email()
.return_once(|| Ok("test@example.com".to_string()));
mock.expect_sign()
.return_once(|_content| Err(SigningError::from_msg("test".to_string())));
let signer = Signer::from(mock);
let fut = SignedUrlBuilder::for_object("projects/_/buckets/b", "o").sign_with(&signer);
assert_send(&fut);
Ok(())
}Thanks!
|
Give me a minute on the test, just fixed the suggested lint issue |
No worries on the test. I can always add it in a follow up PR. Your PR is good to go. Also thanks for doing the CLA business |
|
/gcbrun |
Co-authored-by: Darren Bolduc <dbolduc@google.com>
|
Sorry just saw your comment here -- added the test and ensured it passed, and ran cargo fmt again on the whole file |
Even better. Thanks! |
|
/gcbrun |
|
Let me know if you need anything else like rebase etc. |
This PR updates signed_url.rs to ensure that when canonical_query is created (using a Serializer which is non-send), we drop the Serializer at that point so it doesn't remain alive until the end of the scope.
Currently even though the canonical_query variable is shadowed, rust does not automatically drop the shadowed variable until the end of scope, and
.finish()only takes an&mut Serializer. So the original non-sendcanonical_querylives for the whole scope across an await point and therefore makessign_withnon-send.