RUST-679 Simplify error API to ease pattern matching #301

saghm · 2021-03-09T18:17:54Z

There were several different places in the driver that we assumed we could clone errors, so I handled each of those places (along with cascading changes due to other places related to that code) in a separate commit to make it easier to follow along with each one. The final commit removes the Arc and the Clone derive from the Error type. Because of this, I think the easier way to review this PR is to review each commit separately so that all of the different unrelated changes don't make things confusing. I'll push a separate commit for each change that's suggested during review, and then once everything looks okay, I'll rebase to move each change into the commit associated with the part that changed and request one final review to make sure that all of the changes look okay after that rebase.

saghm · 2021-03-09T18:24:51Z

src/sdam/description/server.rs

@@ -74,7 +73,7 @@ pub(crate) struct ServerDescription {
    // allows us to ensure that only valid states are possible (e.g. preventing that both an error
    // and a reply are present) while still making it easy to define helper methods on
    // ServerDescription for information we need from the isMaster reply by propagating with `?`.
-    pub(crate) reply: Result<Option<IsMasterReply>>,
+    pub(crate) reply: Result<Option<IsMasterReply>, String>,


Because the only place we use the errors on server descriptions is in the error messages to users when server selection fails, the simplest way to avoid needing to copy errors around here is to just store the description of the error instead of the error itself, since that's what will be shown to users anyhow. The rest of the changes in this specific commit are due to this one change.

saghm · 2021-03-09T18:26:04Z

src/sdam/description/topology/mod.rs

@@ -661,8 +666,13 @@ impl TopologyDescription {
    }

    /// Create a new ServerDescription for each address and add it to the topology.
-    fn add_new_servers<'a>(&mut self, servers: impl Iterator<Item = &'a String>) -> Result<()> {
-        let servers: Result<Vec<_>> = servers.map(|server| StreamAddress::parse(server)).collect();
+    fn add_new_servers<'a>(


Rustfmt changed the spacing a bit here, so it might not be super clear what the change is: the return value of the function was changed from Result<()> to Result<(), String>, and then a call to to_string() on the error was added to the iterator expression.

saghm · 2021-03-09T18:28:25Z

src/sdam/description/topology/mod.rs

@@ -731,15 +741,17 @@ pub(crate) struct TopologyDescriptionDiff {
    pub(crate) new_addresses: HashSet<StreamAddress>,
 }

-fn verify_max_staleness(max_staleness: Option<Duration>) -> Result<()> {
+fn verify_max_staleness(max_staleness: Option<Duration>) -> crate::error::Result<()> {


There were two existing uses of this function before this commit, one of which does return the error to users, so we still want to be able to return an actual Error rather than a string in this case. I split this function into two so that we don't have to pattern match the error and assert that it's an ArgumentError (when we know it always will be) when it's called in a place where we don't return the error to users.

src/cursor/common.rs

saghm · 2021-03-09T18:55:56Z

src/sdam/state/server.rs

@@ -74,7 +74,10 @@ impl Server {
 /// TODO: add success cases from application handshakes.
 #[derive(Debug)]
 pub(crate) enum ServerUpdate {
-    Error { error: Error, error_generation: u32 },
+    Error {
+        error: String,


Passed an owned error over the channel is no longer possible since the error also needs to be returned to the user in some places (e.g. when a new connection is created but fails to establish during a checkout). Luckily, the other side of the channel only uses the errors to mark the server as unknown in the topology, and an earlier commit in this PR already updated ServerDescription to use String instead of Error, so we can just pass the string across the channel eagerly instead of lazily getting the message on the other end.

saghm · 2021-03-09T18:57:16Z

src/error.rs

 #[error(display = "{}", kind)]
 #[non_exhaustive]
 pub struct Error {
    /// The type of error that occurred.
-    pub kind: Arc<ErrorKind>,
+    pub kind: ErrorKind,


This is the only substantive change in this commit; the rest are just fixes for the Arc being removed

patrickfreed

Overall looks good! I have one question about possibly preserving Clone and a few other minor questions. Also, it looks like this broke the async-std tests + lint.

patrickfreed · 2021-03-10T17:09:32Z

src/client/executor.rs

@@ -108,7 +108,7 @@ impl Client {

                // Retryable writes are only supported by storage engines with document-level
                // locking, so users need to disable retryable writes if using mmapv1.
-                if let ErrorKind::CommandError(ref err) = err.kind.as_ref() {
+                if let ErrorKind::CommandError(ref err) = err.kind {


is there any reason we would want kind to be a method instead of accessing the field directly? This is what std::io::Error does, but I don't really have any further justification than that.

The main benefit would be that we would be able to change the way that we store the field in the struct without breaking API. As it is currently, removing or changing the type of the field would break user code that accesses the field directly.

I don't really have any strong feelings on this either way, if you think that protection is worthwhile then I'd say go ahead and make the change and if not, seems fine to me to leave it as is.

src/runtime/acknowledged_message.rs

patrickfreed · 2021-03-10T17:14:33Z

src/error.rs

@@ -23,12 +23,12 @@ pub type Result<T> = std::result::Result<T, Error>;
 /// An error that can occur in the `mongodb` crate. The inner
 /// [`ErrorKind`](enum.ErrorKind.html) is wrapped in an `Arc` to allow the errors to be
 /// cloned.
-#[derive(Clone, Debug, Error)]


Before we go through with removing the Clone implementation altogether, have we revisited just wrapping the std::io::Error branch in an Arc to preserve Clone on our type? I went back to see why we didn't opt for this originally, and it seems to be to avoid having users handle the indirection of Arc non-uniformally. For std::io::Error specifically, I'm not sure if Arc actually introduces any additional work for users though, since they have to invoke a method on the error (e.g. kind()) to get any useful result anyways.

I've looked over the other branches of our ErrorKind, and the only ones that also don't implement Clone are our BSON errors (which we own and can make Clone) and TokioTimeoutElapsed error, which we don't actually even use since RUNTIME.timeout converts everything to std::io::Error anyways.

I think if we manage to keep Clone alongside the changes made here to ease in matching, we'll have the best of both worlds.

I considered that a bit too when working on this PR. I didn't end up making the change (although I hadn't looked back into why we didn't choose that originally), although a bit of that did make it into one of the proposals I made in the google doc in the context of making CommandFailedEvent use an alternate error struct.

I guess my main question is what benefit we think users will get from having our error type implement Clone. The only thing I can think of is the reason that we wanted it in the first place (to make their own wrapping error type cloneable), but I don't think this would be quite as common an issue for our users due to the fact that most of them are using the driver in the context of an application rather than a library, and the current consensus in the Rust community is that applications should just use Box<dyn Error> (or something like anyhow to abstract that). While there are some libraries out there that wrap the driver (and obviously more could be written in the future), my general instinct is to make any performance costs (e.g. heap allocations and reference counts) opt-in for users who need them rather than on by default with no way to opt out of, especially since the number of users who will need it in this case are far fewer than the ones who won't. My opposition to doing this isn't particularly strong, though.

I can't really think of a specific example of what a user might need the cloning for, but I imagine it could be something similar to what we needed clone for as you mentioned. Even if they don't require the cloning (as we didn't), having to update their existing code could be non-trivial, as is seen in this PR. Also, if they really do need cloning, they'll need to wrap our entire error in an Arc, which is pretty annoying to deal with (hence this PR 🙂), so even if it is somewhat uncommon, the frustration felt could be high and worth avoiding. Lastly, if we don't go for Clone now, I don't think we'll be able to do so until 3.0, so if it does turn out to be a burden for users, we can't really give them any good fix.

Regarding performance costs, I definitely agree, but given that this Arc would only be allocated in single specific error case (i.e. not the "hot" path), I don't think it's much of a concern.

So overall, for reasons of reducing user friction + safeguarding us against any future need, I think we should try to preserve Clone here.

This also has made me wonder, if we preserve Clone, could we avoid having to make the String changes in this PR? I realize this is an frustrating question to ask after all the work has been done and I'm really sorry about that; it's just that up to this point I had assumed that preserving Clone wasn't going to be possible or easy but now have realized there could be another option.

If we kept Error being Clone, then yes, I think we would want to undo the changes to CommandFailedEvent. All of the other changes are probably fine to keep though, since they don't put any burden on the user and are marginal perf improvements.

src/error.rs

patrickfreed

LGTM! I think this will make our error API a lot nicer to use.

patrickfreed · 2021-03-18T00:22:46Z

Looks like clippy needs to be satisfied but otherwise LGTM (I don't need to re-review the clippy fix)

saghm · 2021-03-18T17:00:20Z

Looks like the clippy issue is just a slight memory inefficiency in the CMAP tests, which isn't really anything to worry about, so I suppressed it.

…rors

saghm force-pushed the RUST-679 branch 4 times, most recently from 7a29c60 to 7361e48 Compare March 9, 2021 18:53

saghm commented Mar 9, 2021

View reviewed changes

saghm marked this pull request as ready for review March 9, 2021 18:57

saghm requested review from isabelatkinson and patrickfreed March 9, 2021 18:57

saghm force-pushed the RUST-679 branch from 7361e48 to a1f36a8 Compare March 9, 2021 22:51

patrickfreed reviewed Mar 10, 2021

View reviewed changes

saghm force-pushed the RUST-679 branch from 9d7ec36 to 538c0eb Compare March 15, 2021 16:53

patrickfreed reviewed Mar 15, 2021

View reviewed changes

src/error.rs Outdated Show resolved Hide resolved

saghm changed the title ~~RUST-679 save error messages rather than strings for internal SDAM errors~~ RUST-679 Simplify error API to ease pattern matching Mar 17, 2021

patrickfreed approved these changes Mar 18, 2021

View reviewed changes

isabelatkinson approved these changes Mar 18, 2021

View reviewed changes

saghm force-pushed the RUST-679 branch 3 times, most recently from 7bdc777 to c8f89a8 Compare March 22, 2021 17:50

saghm added 5 commits March 24, 2021 11:42

RUST-679 save error messages rather than strings for internal SDAM er…

1de54b8

…rors

RUST-679 Use error references to update topology

97847d5

RUST-679 avoid cloning errors in cursor implementation

144a15e

RUST-679 Use error strings when marking servers as unknown from CMAP

77df6e6

RUST-679 Remove inner Arc from Error

9358eaa

saghm force-pushed the RUST-679 branch from c8f89a8 to 9358eaa Compare March 24, 2021 15:42

isabelatkinson mentioned this pull request Mar 24, 2021

RUST-52 Implement Sessions API #304

Merged

saghm merged commit 9225dab into mongodb:master Mar 24, 2021

saghm deleted the RUST-679 branch March 24, 2021 18:40

patrickfreed mentioned this pull request Jul 28, 2022

RUST-1373 Update unified test format runner to support SDAM integration tests #712

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUST-679 Simplify error API to ease pattern matching #301

RUST-679 Simplify error API to ease pattern matching #301

saghm commented Mar 9, 2021 •

edited

Loading

saghm Mar 9, 2021

saghm Mar 9, 2021

saghm Mar 9, 2021

saghm Mar 9, 2021

saghm Mar 9, 2021

patrickfreed left a comment

patrickfreed Mar 10, 2021

saghm Mar 10, 2021

patrickfreed Mar 11, 2021

patrickfreed Mar 10, 2021

saghm Mar 10, 2021

patrickfreed Mar 11, 2021

saghm Mar 11, 2021

patrickfreed left a comment

patrickfreed commented Mar 18, 2021

saghm commented Mar 18, 2021 •

edited

Loading

RUST-679 Simplify error API to ease pattern matching #301

RUST-679 Simplify error API to ease pattern matching #301

Conversation

saghm commented Mar 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickfreed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickfreed left a comment

Choose a reason for hiding this comment

patrickfreed commented Mar 18, 2021

saghm commented Mar 18, 2021 • edited Loading

saghm commented Mar 9, 2021 •

edited

Loading

saghm commented Mar 18, 2021 •

edited

Loading