feat: handlers and dispatchers for get_bso and put_bso

[philbooth]

I spent a bit of time today working on dispatchers for get_bso and put_bso. Although they broadly seem to work (they compile! 😄), I'm not very happy with the code as it stands. As usual I had some trouble bending Rust to my will, hence I'm opening a WIP PR in the hope that you guys can help me with some async pointers on what I could be doing better.

The main problem I struggled with was extracting the common code from the two dispatcher bodies out to a separate method. As you'll see in the diff, there is a fair amount of duplicate code in each body and the indentation got pretty crazy as it wouldn't let me flatten the combinator chains.

I tried two things to fix it:

1. A helper method on DBExecutor that returns MutexGuard<DBManager>:

   impl DBExecutor {
       fn lock(&self, user_id: &str) -> Result<MutexGuard<DBManager>, Error> {
           self
               .db_handles
               .read()
               .map_err(|error| Error::new(ErrorKind::Other, "db handles lock error"))
               .and_then(|db_handles| {
                   db_handles
                       .get(user_id)
                       .ok_or_else(|| Error::new(ErrorKind::NotFound, "unknown user"))
               })
               .and_then(|mutex| {
                   mutex
                       .lock()
                       .map_err(|error| Error::new(ErrorKind::Other, "db manager mutex error"))
               })
       }
   }

   That complains about the lifetime of db_handles:

   error[E0597]: `db_handles` does not live long enough
     --> src/dispatcher.rs:59:17
      |
   59 |                 db_handles
      |                 ^^^^^^^^^^ borrowed value does not live long enough
   ...
   67 |             })
      |             - borrowed value only lives until here
      |
   note: borrowed value must be valid for the anonymous lifetime #1 defined on the method body at 53:5...
     --> src/dispatcher.rs:53:5
      |
   53 | /     fn lock(&self, user_id: &str) -> Result<MutexGuard<DBManager>, Error> {
   54 | |         self
   55 | |             .db_handles
   56 | |             .read()
   ...  |
   67 | |             })
   68 | |     }
      | |_____^
   

2. Variation on the above, this time the helper method takes an FnOnce for running the action code before db_handles goes out of scope:

   impl DBExecutor {
       fn lock<R>(&self, user_id: &str, action: &FnOnce(MutexGuard<DBManager>) -> Result<R, Error>) -> Result<R, Error> {
           self
               .db_handles
               .read()
               .map_err(|error| Error::new(ErrorKind::Other, "db handles lock error"))
               .and_then(|db_handles| {
                   db_handles
                       .get(user_id)
                       .ok_or_else(|| Error::new(ErrorKind::NotFound, "unknown user"))
                       .and_then(|mutex| {
                           mutex
                               .lock()
                               .map_err(|error| Error::new(ErrorKind::Other, "db manager mutex error"))
                               .and_then(action)
                       })
               })
       }
   }

   ...which complained about unsatisfied FnOnce trait bounds:

   error[E0277]: the trait bound `for<'r> std::ops::FnOnce(std::sync::MutexGuard<'r, db::models::DBManager>) -> std::result::Result<R, std::io::Error>: std::ops::Fn<(std::sync::MutexGuard<'_, db::models::DBManager>,)>` is not satisfied
     --> src/dispatcher.rs:66:30
      |
   66 |                             .and_then(action)
      |                              ^^^^^^^^ the trait `std::ops::Fn<(std::sync::MutexGuard<'_, db::models::DBManager>,)>` is not implemented for `for<'r> std::ops::FnOnce(std::sync::MutexGuard<'r, db::models::DBManager>) -> std::result::Result<R, std::io::Error>`
      |
      = note: required because of the requirements on the impl of `std::ops::FnOnce<(std::sync::MutexGuard<'_, db::models::DBManager>,)>` for `&for<'r> std::ops::FnOnce(std::sync::MutexGuard<'r, db::models::DBManager>) -> std::result::Result<R, std::io::Error>`
   

(also the error handling is a hack right now)

Any suggestions @bbangert / @pjenvey / @rfk?

[pjenvey]

Hey Phil, sorry I didn't see this until later in the day. I don't have a good answer yet =] Ben and I will take a closer look tomorrow (our time)

(Also I turned on slack notifications on this repo to #push-dev-bots so we'll get nagged there immediately when PRs open)

[philbooth]

sorry I didn't see this until later in the day.

No worries, I should have mentioned in IRC too, wasn't thinking!

[philbooth]

In the cold light of day this was pretty straightforward to resolve (quelle surprise). The second approach works fine if I use Fn instead of FnOnce and clone the msg struct inside closure, so I've pushed again with that.

I also wrote some integration tests for the server stuff, currently they just assert that the routes don't fail with a valid uid and do fail with an invalid one. I tried to write a complete round-trip test that asserts the result of get_bso matches what I put in with put_bso, but I couldn't get it to succeed yet. That is obviously quite a major hole in the test coverage and the implementation, so I'll keep debugging there to figure out what's up. In the meantime, I thought this PR might still be worthy of consideration.

Note that although it looks like travis has failed on this PR, if you look at travis for my fork the tests are green:

https://travis-ci.org/philbooth/syncstorage-rs/builds/408578028

(and the tests pass locally of course)

Not sure what's up with the mozilla-services travis build, it fails pretty early trying to unpack the parking_lot crate.

$ cargo build --verbose
error: unable to get packages from source
Caused by:
  failed to unpack package `parking_lot v0.5.5`
Caused by:
  failed to unpack entry at `parking_lot-0.5.5/src/condvar.rs`
Caused by:
  failed to unpack `/home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/parking_lot-0.5.5/src/condvar.rs`

I don't have permission to clear the caches on the build unfortunately but I usually find that fixes this kind of thing, if someone who has the authority wants to give it a try.

Anyway, @pjenvey / @bbangert r?