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.

Sign up for GitHub

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

Use spawn_blocking to increase request throughput #243

Merged
merged 3 commits into from
Jul 20, 2021
Merged

Use spawn_blocking to increase request throughput #243

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
82916d2
Use spawn_blocking
giarc3 Jul 20, 2021
bf1d594
Update src/internal/document_api.rs
giarc3 Jul 20, 2021
3d02d12
non_exhaustive IronOxideErr, CHANGELOG
giarc3 Jul 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/document.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,7 +418,7 @@ impl DocumentOps for crate::IronOxide {
add_optional_timeout(
document_api::decrypt_document(
self.device.auth(),
&self.recrypt,
self.recrypt.clone(),
self.device.device_private_key(),
encrypted_document,
),
Expand Down
9 changes: 9 additions & 0 deletions src/internal.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,9 @@ quick_error! {
OperationTimedOut{operation: SdkOperation, duration: std::time::Duration} {
display("Operation {} timed out after {}ms", operation, duration.as_millis())
}
JoinError(err: tokio::task::JoinError) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the addition of this error type means we should do a minor version bump.

Copy link
Member

Choose a reason for hiding this comment

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

https://doc.rust-lang.org/cargo/reference/semver.html#enum-variant-new looks like major is actually the recommendation for adding more variants to an enum, should probably do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a cool page @skeet70, hadn't seen that before. It makes sense that it's considered a breaking change because someone matching over it would need to include the new type for the match to be exhaustive. Should we do a major bump or work to avoid that? If someone has a recommendation for an existing variant that works, I could use that.

If we do bump major, we could consider adding #[non_exhaustive] onto it so that we're free to add new variants in the future without a major bump.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was also wondering about non_exhaustive. I also realized, looking at this, that we would be adding a tokio error to our public API. We already expose error from ring and protobuf, but maybe we should stop doing that.

The chances of a consumer enumerating all of these error types in a match is effectively zero, so maybe allowing non-exhaustive would be fine.

Copy link
Member

Choose a reason for hiding this comment

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

https://doc.rust-lang.org/reference/attributes/type_system.html#the-non_exhaustive-attribute for reference. In TSP code recently I wrote that matches! that enumerates the couple that mattered to me there and ignored the rest. AFAIK non_exhaustive would just always force people to do something with the "rest" even if they've enumerated everything that exists right now. I don't think that would've affected my code. It could be a minor annoyance if you really did want to be exhaustive (and know you were) but I can't think of a use case where you wouldn't be able to have some catch all fallback at the application level.

https://rust-lang.github.io/rfcs/2008-non-exhaustive.html#enums the RFC for it explicitly called out error types being the most common use case.

I think we make this non_exhaustive.

source(err)
}
}
}

Expand Down Expand Up @@ -230,6 +233,12 @@ impl From<recrypt::nonemptyvec::NonEmptyVecError> for IronOxideErr {
}
}

impl From<tokio::task::JoinError> for IronOxideErr {
fn from(e: tokio::task::JoinError) -> Self {
IronOxideErr::JoinError(e)
}
}

const NAME_AND_ID_MAX_LEN: usize = 100;

/// Validate that the provided id is valid for our user/document/group IDs. Validates that the
Expand Down
40 changes: 22 additions & 18 deletions src/internal/document_api.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1055,31 +1055,35 @@ pub async fn document_update_bytes<

/// Decrypt the provided document with the provided device private key. Return metadata about the document
/// that was decrypted along with its decrypted bytes.
pub async fn decrypt_document<CR: rand::CryptoRng + rand::RngCore>(
pub async fn decrypt_document<CR: rand::CryptoRng + rand::RngCore + Send + Sync + 'static>(
auth: &RequestAuth,
recrypt: &Recrypt<Sha256, Ed25519, RandomBytes<CR>>,
recrypt: std::sync::Arc<Recrypt<Sha256, Ed25519, RandomBytes<CR>>>,
device_private_key: &PrivateKey,
encrypted_doc: &[u8],
) -> Result<DocumentDecryptResult, IronOxideErr> {
let (doc_header, mut enc_doc) = parse_document_parts(encrypted_doc)?;
let doc_meta = document_get_metadata(auth, &doc_header.document_id).await?;
let sym_key = transform::decrypt_as_symmetric_key(
recrypt,
doc_meta.0.encrypted_symmetric_key.clone().try_into()?,
device_private_key.recrypt_key(),
)?;
let device_private_key = device_private_key.clone();
tokio::task::spawn_blocking(move || {
let sym_key = transform::decrypt_as_symmetric_key(
&(*recrypt.clone()),
giarc3 marked this conversation as resolved.
Show resolved Hide resolved
doc_meta.0.encrypted_symmetric_key.clone().try_into()?,
device_private_key.recrypt_key(),
)?;

Ok(
aes::decrypt(&mut enc_doc, *sym_key.bytes()).map(move |decrypted_doc| {
DocumentDecryptResult {
id: doc_meta.0.id,
name: doc_meta.0.name,
created: doc_meta.0.created,
updated: doc_meta.0.updated,
decrypted_data: decrypted_doc.to_vec(),
}
})?,
)
Ok(
aes::decrypt(&mut enc_doc, *sym_key.bytes()).map(move |decrypted_doc| {
DocumentDecryptResult {
id: doc_meta.0.id,
name: doc_meta.0.name,
created: doc_meta.0.created,
updated: doc_meta.0.updated,
decrypted_data: decrypted_doc.to_vec(),
}
})?,
)
})
.await?
}

/// Decrypt the unmanaged document. The caller must provide both the encrypted data as well as the
Expand Down
10 changes: 7 additions & 3 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,11 @@ use rand::{
};
use rand_chacha::ChaChaCore;
use recrypt::api::{Ed25519, RandomBytes, Recrypt, Sha256};
use std::{convert::TryInto, fmt, sync::Mutex};
use std::{
convert::TryInto,
fmt,
sync::{Arc, Mutex},
};
use vec1::Vec1;

/// A `Result` alias where the Err case is `IronOxideErr`
Expand Down Expand Up @@ -259,7 +263,7 @@ pub mod config {
/// performed in the context of the account provided.
pub struct IronOxide {
pub(crate) config: IronOxideConfig,
pub(crate) recrypt: Recrypt<Sha256, Ed25519, RandomBytes<recrypt::api::DefaultRng>>,
pub(crate) recrypt: Arc<Recrypt<Sha256, Ed25519, RandomBytes<recrypt::api::DefaultRng>>>,
/// Master public key for the user identified by `account_id`
pub(crate) user_master_pub_key: PublicKey,
pub(crate) device: DeviceContext,
Expand Down Expand Up @@ -431,7 +435,7 @@ impl IronOxide {
) -> IronOxide {
IronOxide {
config: config.clone(),
recrypt: Recrypt::new(),
recrypt: Arc::new(Recrypt::new()),
device: device_context.clone(),
user_master_pub_key: curr_user.user_public_key().to_owned(),
rng: Mutex::new(ReseedingRng::new(
Expand Down