feat: fix asserts on client dis/connect and convert inc call to rust #1178

bbangert · 2018-04-15T23:47:12Z

Removes the asserts when adding the uaid to the uaids hash, and issue
proper disconnect call to existing client. Ensure uaid removal removes
the right client indicated by a separate UUID per client object.

Convert IncrementStorage call to pure Rust async and remove Python
implementation.

Closes #1177

jrconlin · 2018-04-15T23:57:13Z

autopush_rs/src/client.rs

+    fn process_increment_storage(&mut self) -> ClientState {
+        // Let the variable copies begin, so that nothing is left dangling in the future
+        // we then assemble because Lifetimes.
+        let webpush = self.webpush.as_ref().unwrap();


I'm guessing that these unwraps are gets trapped up by client.transition()? Should we call these out as specific errors if they fail?

Logically to get to the point where we can do state transitions, we need a webpush here, same with how the state machine guarantee's we'll have unacked by this point or we wouldn't need to increment storage.

jrconlin · 2018-04-16T00:03:07Z

autopush_rs/src/server/mod.rs

@@ -338,9 +342,19 @@ impl Server {
        } else {
            ServiceChangeTracker::new(Vec::new())
        };
+        let region = if env::var("AWS_PROFILE").unwrap_or("default".to_string()) == "test" {


use if cfg!(test) here?

Do we set what ever that checks somewhere?

cfg!() can be called at run time to see what configuration you happen to be in. It's the inline equivalent of "#[cfg(test)]".

It might be more useful than checking the AWS_PROFILE for a test configuration value.

I mean, how do we flag in the environment that its in test mode? It's not running a Rust based test suite that would flag this afaik.

Right, the #cfg test toggle is via a compiler flag, so unfortunately we can't use it here.

The python code keys on AWS_LOCAL_DYNAMODB, so I'd rather this key on that as well. Then I don't think the changes to .travis.yml/boto.cfg/tox.ini are necessary (and local testing should continue to work as normal)

codecov-io · 2018-04-16T00:31:56Z

Codecov Report

Merging #1178 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #1178   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          60      60           
  Lines       10078   10078           
======================================
  Hits        10078   10078

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1e98d2...a481959. Read the comment docs.

pjenvey · 2018-04-16T18:51:53Z

autopush_rs/src/server/mod.rs

        debug!("Disconnecting client!");
        let mut uaids = self.uaids.borrow_mut();
-        uaids.remove(uaid).expect("uaid not registered");
+        let client_exists = if let Some(client) = uaids.get(uaid) {


nitpick: could go with a one liner: let client_exists = uaids.get(uaid).map(|client| client.uid == *uid).unwrap_or(false);

pjenvey · 2018-04-16T18:51:58Z

autopush_rs/src/util/ddb_helpers.rs

+/// A bunch of macro helpers from rusoto_helpers code, which they pulled from crates.io because
+/// they were waiting for rusuto to hit 1.0.0 or something. For sanity, they are instead accumulated
+/// here for our use.
+macro_rules! attributes {


these first 3 should have #[allow(unused_macros)] for now to quiet rustc warnings

pjenvey · 2018-04-16T23:06:43Z

autopush_rs/src/server/mod.rs

@@ -338,9 +342,19 @@ impl Server {
        } else {
            ServiceChangeTracker::new(Vec::new())
        };
+        let region = if env::var("AWS_PROFILE").unwrap_or("default".to_string()) == "test" {


Right, the #cfg test toggle is via a compiler flag, so unfortunately we can't use it here.

The python code keys on AWS_LOCAL_DYNAMODB, so I'd rather this key on that as well. Then I don't think the changes to .travis.yml/boto.cfg/tox.ini are necessary (and local testing should continue to work as normal)

pjenvey · 2018-04-16T23:08:43Z

autopush_rs/src/server/mod.rs

+                name: "local_dynamodb".to_string(),
+            }
+        } else {
+            Region::UsEast1


don't hardcode, you want Region::default() so it's read from the AWS_DEFAULT_REGION env var

Removes the asserts when adding the uaid to the uaids hash, and issue proper disconnect call to existing client. Ensure uaid removal removes the right client indicated by a separate UUID per client object. Convert IncrementStorage call to pure Rust async and remove Python implementation. Closes #1177

pjenvey · 2018-04-17T01:05:31Z

autopush_rs/src/server/mod.rs

@@ -338,9 +342,17 @@ impl Server {
        } else {
            ServiceChangeTracker::new(Vec::new())
        };
+        let region = env::var("AWS_LOCAL_DYNAMODB").map(|endpoint| {


nitpick: unwrap_or_else would be preferred here (don't care much because this is for tests)

But it needs to construct the Region object with the value?

for the fallback I mean (oops I annotated the wrong line). it's just the lazy version of unwrap_or

.unwrap_or_else(|| Region::default());

unwrap_or_else(Region::default) even!

pjenvey · 2018-04-17T01:07:02Z

.travis.yml

@@ -28,7 +28,7 @@ install:
 - pip install tox ${CODECOV:+codecov}
 - if [ ${WITH_RUST:-true} != "false" ]; then curl https://sh.rustup.rs | sh -s -- -y || travis_terminate 1; fi
 - export PATH=$PATH:$HOME/.cargo/bin
- export AWS_SHARED_CREDENTIALS_FILE=./automock/credentials.cfg
+- export AWS_SHARED_CREDENTIALS_FILE=./automock/boto.cfg


is this and the boto.cfg change still necessary? (boto.cfg should probably be killed at this point if anything)

It is, because rusoto wants to find itself some aws access/secret keys, and if it can't find them in a file it expects, it starts issuing HTTP calls looking for IAM keys and such which messes things up. This is also why I added random access/secret keys to the boto.cfg. (There is no credentials.cfg)

but credentials.cfg has them

What credentials.cfg? https://github.com/mozilla-services/autopush/tree/master/automock

bbangert requested review from pjenvey and jrconlin April 15, 2018 23:47

bbangert added the in progress label Apr 15, 2018

bbangert force-pushed the feat/issue-1177 branch from a9b6fc9 to c1756f3 Compare April 15, 2018 23:51

jrconlin reviewed Apr 16, 2018

View reviewed changes

jrconlin previously approved these changes Apr 16, 2018

View reviewed changes

pjenvey reviewed Apr 16, 2018

View reviewed changes

bbangert dismissed jrconlin’s stale review via 2c45ec9 April 16, 2018 23:00

bbangert force-pushed the feat/issue-1177 branch from c1756f3 to 2c45ec9 Compare April 16, 2018 23:00

pjenvey suggested changes Apr 16, 2018

View reviewed changes

bbangert force-pushed the feat/issue-1177 branch from 2c45ec9 to b15f89a Compare April 17, 2018 00:44

bbangert force-pushed the feat/issue-1177 branch from b15f89a to a481959 Compare April 17, 2018 01:01

pjenvey approved these changes Apr 17, 2018

View reviewed changes

bbangert merged commit b9cc703 into master Apr 17, 2018

bbangert deleted the feat/issue-1177 branch April 17, 2018 02:08

bbangert removed the in progress label Apr 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: fix asserts on client dis/connect and convert inc call to rust #1178

feat: fix asserts on client dis/connect and convert inc call to rust #1178

bbangert commented Apr 15, 2018

jrconlin Apr 15, 2018

bbangert Apr 16, 2018

jrconlin Apr 16, 2018

bbangert Apr 16, 2018

jrconlin Apr 16, 2018

bbangert Apr 16, 2018

pjenvey Apr 16, 2018

codecov-io commented Apr 16, 2018 •

edited

Loading

pjenvey Apr 16, 2018

pjenvey Apr 16, 2018

pjenvey Apr 16, 2018

pjenvey Apr 16, 2018

pjenvey Apr 17, 2018

bbangert Apr 17, 2018

pjenvey Apr 17, 2018

pjenvey Apr 17, 2018

pjenvey Apr 17, 2018 •

edited

Loading

bbangert Apr 17, 2018 •

edited

Loading

pjenvey Apr 17, 2018

bbangert Apr 17, 2018

feat: fix asserts on client dis/connect and convert inc call to rust #1178

feat: fix asserts on client dis/connect and convert inc call to rust #1178

Conversation

bbangert commented Apr 15, 2018

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

codecov-io commented Apr 16, 2018 • edited Loading

Codecov Report

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

pjenvey Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

bbangert Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Apr 16, 2018 •

edited

Loading

pjenvey Apr 17, 2018 •

edited

Loading

bbangert Apr 17, 2018 •

edited

Loading