Use spawn_blocking to increase request throughput #243
Conversation
src/internal.rs
Outdated
| @@ -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) { | |||
There was a problem hiding this comment.
I think the addition of this error type means we should do a minor version bump.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Co-authored-by: Clint Frederickson <[email protected]>
Description in #241 (comment) by @skeet70: