feat: add megaphone broadcast handling, ping check, server tracking #1152

bbangert · 2018-03-13T01:10:55Z

Handles broadcast subscriptions per client, holds a single broadcast
change set tracker, and sends out broadcasts at the ping interval to
clients that have new broadcast deltas to send.

Issue #1129

Note: Only remaining work is to add the megaphone polling portion that grabs the broadcast master list and keeps it up to date.

codecov-io · 2018-03-13T01:53:13Z

Codecov Report

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

@@          Coverage Diff           @@
##           master   #1152   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          60      60           
  Lines        9940    9940           
======================================
  Hits         9940    9940

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 1b0be54...1fe4eeb. Read the comment docs.

alexcrichton

Looks great!

alexcrichton · 2018-03-13T14:12:48Z

autopush_rs/src/client.rs

@@ -570,15 +615,18 @@ where
        rotate_message_table: bool,
        check_storage: bool,
        connected_at: u64,
+        services: &Vec<Service>,


FWIW &[T] tends to be more idiomatic rather than &Vec<T>

Ah, is that used only when its a reference, or just in general as its shorter than Vec<T>?

He just meant for &Vec. If you're not doing anything too Vec specific, more types are able to deref into &[T] so it's good practice to prefer the slice type. I just linked this doc so, here's its further explanation https://deterministic.space/elegant-apis-in-rust.html#use-conversion-traits

bbangert · 2018-03-13T14:55:29Z

autopush_rs/src/client.rs

@@ -479,7 +524,7 @@ where
                    Either::A(ClientMessage::Nack { .. }) => {
                        self.data.srv.metrics.incr("ua.command.nack").ok();
                        self.data.webpush.as_mut().unwrap().stats.nacks += 1;
-                        ClientState::WaitingForAcks
+                        ClientState::Await


This was a bug I noticed and fixed here, as its supposed to return to the prior state.

jrconlin · 2018-03-13T15:29:57Z

autopush_rs/src/client.rs

@@ -422,6 +439,20 @@ where
                    return Ok(next_state.into());
                }
                match try_ready!(self.data.input_or_notif()) {
+                    Either::A(ClientMessage::BroadcastSubscribe { broadcasts }) => {


nit: "broadcasts" is being used as a parameter, local variable and key here. It's a bit confusing.

It's used to destructure, then its re-bound locally as the Vec, and passed into a function. I can remove an intermediary reassignment though and pass the resulting Vec directly to the single use of it in a function, that might clear it up some.

pjenvey · 2018-03-13T19:25:15Z

autopush_rs/src/server/mod.rs

@@ -659,8 +702,20 @@ impl<T> WebpushSocket<T> {
        Error: From<T::SinkError>,
    {
        if self.ping {
-            debug!("sending a ping");
-            match self.inner.start_send(Message::Ping(Vec::new()))? {
+            let mut msg = Message::Ping(Vec::new());


easy mut kill here: let msg = if let etc else Message::Ping(Vec::new())

pjenvey · 2018-03-13T19:33:18Z

autopush_rs/src/server/mod.rs

+                        self.waiting = WaitingFor::SendPing;
+                        socket.broadcast_delta = None;
+                    } else {
+                        let at = Instant::now() + self.srv.opts.auto_ping_timeout;


these 2 lines are duplicated in both cases

pjenvey · 2018-03-13T19:38:52Z

autopush_rs/src/util/megaphone.rs

+        val.into_iter().map(|v| v.into()).collect()
+    }
+
+    pub fn to_hashmap(service_vec: Vec<Service>) -> HashMap<String, String> {


this function consumes service_vec so one of the rust naming conventions prefers it called into_hashmap https://deterministic.space/elegant-apis-in-rust.html#more-method-name-conventions

pjenvey · 2018-03-13T19:54:16Z

autopush_rs/src/client.rs

@@ -570,15 +615,18 @@ where
        rotate_message_table: bool,
        check_storage: bool,
        connected_at: u64,
+        services: &Vec<Service>,


He just meant for &Vec. If you're not doing anything too Vec specific, more types are able to deref into &[T] so it's good practice to prefer the slice type. I just linked this doc so, here's its further explanation https://deterministic.space/elegant-apis-in-rust.html#use-conversion-traits

pjenvey · 2018-03-13T19:59:14Z

autopush_rs/src/util/megaphone.rs

+    /// Update a `ClientServices` to account for a new service.
+    ///
+    /// Returns services that have changed.
+    pub fn client_service_add_service(&self, client_service: &mut ClientServices, services: &[Service]) -> Option<Vec<Service>> {


long line! (we could use cargo fmt run, separate commit though)

pjenvey · 2018-03-13T20:01:41Z

autopush_rs/src/util/megaphone.rs

+    /// Returns services that have changed.
+    pub fn client_service_add_service(&self, client_service: &mut ClientServices, services: &[Service]) -> Option<Vec<Service>> {
+        let mut svc_delta = self.change_count_delta(client_service).unwrap_or(Vec::new());
+        for svc in services.iter() {


off hand i don't know if it's easily refactorable/doable but: service_delta has a very similar loop here

Indeed, they both need to do slightly different things though, so I didn't see any way to abstract it out.

Handles broadcast subscriptions per client, holds a single broadcast change set tracker, and sends out broadcasts at the ping interval to clients that have new broadcast deltas to send. Issue #1129

bbangert assigned jrconlin Mar 13, 2018

bbangert requested review from pjenvey and alexcrichton March 13, 2018 01:10

bbangert unassigned jrconlin Mar 13, 2018

bbangert added the in progress label Mar 13, 2018

bbangert requested a review from jrconlin March 13, 2018 01:11

bbangert force-pushed the feat/issue-1129.2 branch from 12573a5 to b5c332a Compare March 13, 2018 01:16

alexcrichton previously approved these changes Mar 13, 2018

View reviewed changes

bbangert commented Mar 13, 2018

View reviewed changes

jrconlin reviewed Mar 13, 2018

View reviewed changes

bbangert dismissed alexcrichton’s stale review via f930c12 March 13, 2018 15:43

bbangert force-pushed the feat/issue-1129.2 branch 3 times, most recently from df46ebe to edd627a Compare March 13, 2018 17:04

jrconlin previously approved these changes Mar 13, 2018

View reviewed changes

pjenvey suggested changes Mar 13, 2018

View reviewed changes

pjenvey reviewed Mar 13, 2018

View reviewed changes

bbangert dismissed jrconlin’s stale review via 9ec6808 March 13, 2018 21:01

bbangert force-pushed the feat/issue-1129.2 branch from edd627a to 9ec6808 Compare March 13, 2018 21:01

feat: add megaphone broadcast handling, ping check, server tracking

1fe4eeb

Handles broadcast subscriptions per client, holds a single broadcast change set tracker, and sends out broadcasts at the ping interval to clients that have new broadcast deltas to send. Issue #1129

bbangert force-pushed the feat/issue-1129.2 branch from 9ec6808 to 1fe4eeb Compare March 13, 2018 22:14

pjenvey approved these changes Mar 13, 2018

View reviewed changes

bbangert merged commit a98c045 into master Mar 13, 2018

bbangert removed the in progress label Mar 13, 2018

bbangert deleted the feat/issue-1129.2 branch March 13, 2018 22:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add megaphone broadcast handling, ping check, server tracking #1152

feat: add megaphone broadcast handling, ping check, server tracking #1152

bbangert commented Mar 13, 2018

codecov-io commented Mar 13, 2018 •

edited

Loading

alexcrichton left a comment

alexcrichton Mar 13, 2018

bbangert Mar 13, 2018 •

edited

Loading

pjenvey Mar 13, 2018

bbangert Mar 13, 2018

jrconlin Mar 13, 2018

bbangert Mar 13, 2018

pjenvey Mar 13, 2018

pjenvey Mar 13, 2018

pjenvey Mar 13, 2018

pjenvey Mar 13, 2018

pjenvey Mar 13, 2018

pjenvey Mar 13, 2018

bbangert Mar 13, 2018

feat: add megaphone broadcast handling, ping check, server tracking #1152

feat: add megaphone broadcast handling, ping check, server tracking #1152

Conversation

bbangert commented Mar 13, 2018

codecov-io commented Mar 13, 2018 • edited Loading

Codecov Report

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbangert Mar 13, 2018 • 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

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 Mar 13, 2018 •

edited

Loading

bbangert Mar 13, 2018 •

edited

Loading