Presence: Heartbeat #3704

rk-for-zulip · 2019-11-26T19:05:25Z

~~Draft PR to centralize presence-reporting code. Not yet suitable for merge due to, at a minimum, the reasons noted in the final commit.~~

This may not be the heartbeat pattern we actually want, but if not, it should be easy to replace.

[edit by @gnprice: Fixes (or moots) #3699 ; part of addressing #3659 .]

rk-for-zulip · 2019-12-02T22:20:34Z

Commenting to make it unambiguous that this is no longer a draft PR.

(The exact heartbeat pattern desired is still under discussion, but the structure introduced here isolates that, and should be separately reviewable.)

gnprice

Thanks @ray-kraesig !

We had further chat discussion today about the pattern we want, and I think we have a conclusion:

So I think this means that on going into the background, we should

not send any update about that; but

stop sending active updates, for as long as we're continuously in the background.

And when we come back to the foreground, if it's been over a minute, we should immediately send an active update.

92c5578 api [nfc]: rename reportPresence parameter

This looks good! Feel free to merge whenever.

da9b569 presence: strip throttling from reportPresence wrapper

This looks like a good implementation of what it says on the tin. I think it may not be part of what we want following today's discussion, though.

Or rather: we do want throttling, I think. (The setInterval callbacks can accumulate as we switch between accounts, right? Not a good design in itself, but the throttling saves us from the consequences.) I think the idea in this branch is that that'll happen at another layer, which is reasonable. Not sure it simplifies things to pre-emptively take it out of this layer as this separate commit, though.

c196f9d presence: add Heartbeat class (+ tests)
4e2dd81 presence: move all presence-reporting logic into new HeartbeatComponent

I haven't yet read these two in detail, but this general design seems pretty reasonable! Some code comments below.

src/presence/heartbeat.js

src/presence/__tests__/heartbeat-test.js

src/boot/AppEventHandlers.js

src/presence/HeartbeatComponent.js

gnprice · 2019-12-03T02:39:59Z

src/presence/HeartbeatComponent.js

+
+  componentWillUnmount() {
+    AppState.removeEventListener('change', this.onStateChange);
+    this.heartbeat.stop();


This looks asymmetrical. Should we be starting it in componentDidMount?

... Ah I see, we do that conditionally via onStateChange.

How about naming the latter something like updateHeartbeatState? I.e. named for what it does rather than when it's called. There are a couple of call sites that aren't the "state change" callback, and this name is a little puzzling at both of them.

Works for me. (The asymmetry is worth a couple of comments, too.)

gnprice · 2019-12-03T02:48:53Z

src/presence/HeartbeatComponent.js

+// Note that "PureComponent" is of questionable veracity: this component's
+// entire purpose is to emit network calls for their observable side effects.
+// However, it is at least true that there is never a need to call `render()` if
+// the props haven't changed.
+class PresenceHeartbeat extends PureComponent<Props> {


I think that's actually 100% consistent with what PureComponent is intended to mean:
https://reactjs.org/docs/react-api.html#reactpurecomponent

After all, almost all components are meant to cause something to appear in the UI; and any component with a widget like a button (that does something) is capable of causing quite general side effects, often a network request. All of that is independent of being a PureComponent. So the sense of "pure" in PureComponent has to be a lot narrower than "this is side-effect-free code".

I think in fact it really runs only in the dual direction: it says it's free of suffering certain kinds of side effects, but nothing about what side effects it may cause.

It could perhaps have a more lucid name than it does. 🙂

After all, almost all components are meant to cause something to appear in the UI; and any component with a widget like a button (that does something) is capable of causing quite general side effects, often a network request.

This is true, as far as it goes – but PureComponent.render() in particular is expected to do precisely none of those things, and instead only to return a specification of how and when one might later go about doing them.

That expectation is flagrantly violated by this component, whose .render() can, and often will, fire off a network request before returning. It's less pure than many Component.render() functions. The kindest thing one can say about it, I think, is that it is idempotent even when considered as a function from World to World.

(Well, modulo modifications of this and its prototype chain. But if you're breaking the equivalence of this.foo.bind(this) and () => this.foo(), then nothing holds anyway.)

That expectation is flagrantly violated by this component, whose .render() can, and often will, fire off a network request before returning.

Hmm, I see.

I think that's a sign that render isn't the right place to put that call -- quite independently of PureComponent vs. its base class Component.

Consulting the API docs:
https://reactjs.org/docs/react-component.html
I think componentDidUpdate is the intended place to put this kind of call. That's based on its own description, and also on "use this instead" pointers to it in the descriptions of getDerivedStateFromProps and the deprecated UNSAFE_componentWillReceiveProps.

👍

(The docs aren't explicit as to whether componentDidUpdate will fire if the post-render() reconciliation phase says there are no changes, but it seems that it will.)

Oof, that's a regrettable form for documentation to be in. :-/ Glad you tracked it down -- please mention it somewhere so that we, at least, can hang onto that information.

Now that render just returns null, I think this stack pops down to my first comment.

It is now the case that this component performs network calls as a side effect of render(), rather than in render() directly.

Modulo that change, I think this warning remains appropriate.

rk-for-zulip · 2019-12-04T00:40:39Z

So I think this means that on going into the background, we should

not send any update about that; but

stop sending active updates, for as long as we're continuously in the background.

And when we come back to the foreground, if it's been over a minute, we should immediately send an active update.

I'm not entirely convinced that this is the right approach: it seems like the server could synthesize that from the more precise and accurate information stream (i.e., one with "idle" events) with very little overhead.

An implementing commit ~~is nonetheless attached~~ will follow nonetheless, once I have a sane test approach implemented. (And possibly once discussion settles down over on the Zulip channel, since I'm afraid I kicked it back up again.)

Not sure it simplifies things to pre-emptively take [the throttling] out of this layer as this separate commit, though.

It's one less thing in the HeartbeatComponent commit, at least. (I suppose it could be squashed into that one, but I'm not sure that would simplify things, either. ¯\_(ツ)_/¯)

gnprice · 2019-12-04T23:16:36Z

it seems like the server could synthesize that from the more precise and accurate information stream (i.e., one with "idle" events) with very little overhead.

The thing is that "idle" doesn't mean "the user went away just now". Rather it means "hi, still here, but btw the user hasn't interacted with me (this client) recently."

... But really the better place for that design discussion is the chat thread; here we're in danger of overflowing what works well on a GitHub thread, as well as just getting mixed with the details of code. Want to follow up there?

Mobile apps are not usually described as "having focus", and the underlying parameter value is `"active"` anyway.

The throttling logic currently loses, rather than delaying, an `idle` which terminates a stream of `active` presence reports. Remove it. Technically, this alone would fix zulip#3699 as literally reported, since the `idle` report is now emitted. However, the `active`-emitting `setInterval` callback is still present and may still fire off events. (Whether it actually does or not is presumably up to the underlying OS.)

Add `Heartbeat`, a generic-looking class which will be used to perform presence-reporting. Also add tests, to ensure that it behaves as expected.

Centralize all presence-reporting logic into a new zero-display React component, currently (and somewhat arbitrarily) located under AppStateHandlers. This fixes at least two issues: * If the user logs in and out, or otherwise performs an additional "initial fetch" without killing the app, the presence timer will be started multiple times. (This was previously concealed by throttling logic in usersActions.reportPresence, which was itself removed in a previous patch in this set.) * If the user goes idle (e.g. by hitting the Home button), the presence timer will still run, and will still attempt to send `active` notifications. (These attempts are often suppressed while the app is in the background, but there's no guarantee of this.) Fixes zulip#3699 (properly), and is work towards zulip#3659. Unfortunately, there is no user-visible change yet: modifications will be needed on the Zulip server side as well.

Jest doesn't currently (v24.9.0) mock out `Date.now()` when using fake timers. This is expected to change in 25.0 when Jest switches over to using Lolex for its fake timer implementation. We need it today, though, so we'll jump the gun on that.

Although we don't make use of Date.now in the current implementation of Heartbeat, we're about to. Convert its tests to use Lolex, so that Date.now() will be properly faked. Rather than completely rewrite all the timer calls to use Lolex's API directly (since we'd just end up changing them back once we can upgrade Jest), we use a small shim, taken from the code on Jest's current master branch.

Following discussion on chat.zulip.org's #mobile-team channel, adjust the presence-reporting protocol so that it never sends "idle". https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/presence.20and.20notifications

The heartbeat callback is now only ever called with `true`. Simplify the associated code by removing the now-constant argument.

rk-for-zulip · 2019-12-06T01:24:10Z

Updated with new presence protocol, new tests, and a (hopefully very temporary) new dev-dependency.

gnprice

Thanks @ray-kraesig ! Comments below. Nearly all are only about comments and names and such, other than the very last thing I spotted.

One (equally small) comment that's on a commit message and has no natural home in the code:

heartbeat: change presence-reporting protocol

Following discussion on chat.zulip.org's #mobile-team channel, adjust
the presence-reporting protocol so that it never sends "idle".

https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/presence.20and.20notifications

s/channel/stream/ :wink:
Should include a near operator, i.e. link to a specific message -- otherwise for someone caught up this goes to the latest, which could in the future be much later than you intend. (And it goes to the first unread if any, which is probably also less helpful than linking to a specific message at the point where we reached the relevant conclusion.)

gnprice · 2019-12-09T21:29:48Z

src/presence/HeartbeatComponent.js

+  }
+
+  // React to any state change.
+  updateHeartbeatState: () => void = () => {


This line threw me briefly because the two => made it look like it's a nested lambda.

Our usual style would be to write the types at the lambda:

updateHeartbeatState = (): void => {

or actually more typically to not mention the void return type here:

updateHeartbeatState = () => {

gnprice · 2019-12-09T21:35:00Z

src/boot/AppEventHandlers.js

-    return <View style={styles.wrapper}>{this.props.children}</View>;
+    return (
+      <>
+        <HeartbeatComponent />


I think it'd be helpful to have the word "presence" in the name -- that's the Zulip jargon term for this subsystem, so it's the quickest way to make clear what the general purpose of this component is.

... And then I see the class is actually already named PresenceHeartbeat! 😄 That'd be a good name for the module.

gnprice · 2019-12-09T21:37:58Z

src/boot/AppEventHandlers.js

+    return (
+      <>
+        <HeartbeatComponent />
+        <View style={styles.wrapper}>{this.props.children}</View>


Seeing the render () { return null; } style in the new component, it occurs to me it might be helpful -- might make things a bit clearer -- to pull out the rest of the work of AppEventHandlers as a component with a similar null render, to be included as a sibling rather than a parent of the rest of the app.

(As a followup, I think.)

(from offline) More like four or five different sibling components, I think. Agreed otherwise.

gnprice · 2019-12-09T21:41:20Z

src/presence/HeartbeatComponent.js

+// Note that "PureComponent" is of questionable veracity: this component's
+// entire purpose is to emit network calls for their observable side effects.
+// However, it is at least true that there is never a need to call `render()` if
+// the props haven't changed.
+class PresenceHeartbeat extends PureComponent<Props> {


Now that render just returns null, I think this stack pops down to my first comment.

gnprice · 2019-12-09T21:43:15Z

src/presence/HeartbeatComponent.js

+    // N.B.: If `auth` changes, we do not send out a final `false` presence
+    // status for the previous `auth`. It's the responsibility of our logout
+    // handler to determine whether that's necessary.
+    //
+    // (TODO: ensure that our logout handlers actually do that.)


This comment is out of date, right? Especially the "responsibility" / TODO bits. The first bit remains literally true, but is now confusing instead of helpful 🙂

gnprice · 2019-12-09T21:49:00Z

package.json

@@ -108,6 +108,7 @@
    "jest-environment-jsdom": "^24.9.0",
    "jest-environment-jsdom-global": "^1.2.0",
    "jest-extended": "^0.11.2",
+    "lolex": "^5.1.1",


Jest doesn't currently (v24.9.0) mock out `Date.now()` when using fake timers. This is expected to change in 25.0 when Jest switches over to using Lolex for its fake timer implementation.

Oh neat!

We were looking the other day at some Jest threads on this subject, I think, and didn't spot this news then. Where did you find it?

(That would be a helpful link for the commit message.)

Ahh, I see, the next commit has the answer.

Simplest thing may be to just squash the two, as the deps commit is very small.

Also unfortunately my reading of that Jest PR would not be as optimistic as this. AFAICT there's been no reply from anyone at FB in the 10 months that PR has been open. Landing this PR, and doing so for Jest v25, is the author's plan but it's not clear it's anyone else's.

Simplest thing may be to just squash the two, as the deps commit is very small.

Done!

Also unfortunately my reading of that Jest PR would not be as optimistic as this.

The author in question is a collaborator for Jest (i.e., has commit privileges on the repo), so I have somewhat greater hopes of seeing this in for v25.

(The new message for the squashed commit is somewhat more measured, nonetheless.)

gnprice · 2019-12-09T22:29:51Z

src/presence/__tests__/heartbeat-test.js

@@ -55,7 +55,7 @@ class Lolex {
 let lolex: Lolex;

 // type alias for Jest callback functions of type (boolean) => void
-type CallbackType = JestMockFn<$ReadOnlyArray<boolean>, void>;
+type CallbackType = JestMockFn<$ReadOnlyArray<void>, void>;


This comment is out of date 🙂

Simplest fix is to update it in tandem.

But I think this also makes an excellent example of why it's good to avoid repeating details of the code in the comments. Here, the information this is really trying to add is something like "those type arguments are the types of the mock function's parameters and return." One way to say that would be

// Type parameters are: JestMockFn<TArguments: $ReadOnlyArray<*>, TReturn>

Unfortunately * is deprecated. In particular I'm not sure that wouldn't have the effect of ReadOnlyArray<any>, which – while accurate for Jest – is probably not what I want here.

gnprice · 2019-12-09T22:31:47Z

src/presence/__tests__/heartbeat-test.js

+  // Heartbeat erases its callback type (and it should probably be private
+  // anyway), so the `expectRunning` and `expectNotRunning` helpers take the
+  // callback mock as a separate well-typed argument.


Huh, I'm puzzled -- what do you mean by saying it erases its callback type? Here's the definition in the early part of the branch:

class Heartbeat { intervalId: IntervalID | null = null; callback: (state: boolean) => void; milliseconds: number;

and a later commit does s/state: boolean//.

It seems like the type is right there. What am I missing?

That's not the full type, though; it's just the maximal supertype. The "underlying type" of the callback object here is CallbackType.

Not erasing the type would involve giving Heartbeat a generic type parameter T: (boolean) => void (later () => void). This isn't difficult, of course; but it's also pointless unless callback is part of Heartbeat's public interface. (Which it really shouldn't be!)

gnprice · 2019-12-09T22:37:39Z

src/presence/heartbeat.js

+    if (this.previousTime + this.milliseconds <= Date.now()) {
+      this.doCallback();
+    }
+    this.intervalId = setInterval(this.doCallback, this.milliseconds);
+  }


I think this can have a gap of up to twice nominal, if the state flaps shortly before the regular timer would fire.

2 - ε, yes, as mentioned in chat... whereupon we talked a great deal about the possibility of too many presence signals, but much less about the possibility of not enough. :/

After a bit of poking and refactoring... hilariously, it looks like the simplest way to minimize that gap also removes any reason to call Date.now, and therefore makes Lolex useless here. Yay? ¯\_(ツ)_/¯

Addendum: the above was not true, and Lolex remains useful.

gnprice · 2019-12-09T23:11:21Z

BTW I also just added a link in the PR description to #3699, so that that thread gets an autolink back here. I was making a comment there, wanted to refer to this PR, and found I didn't have a reference to it 🙂

rk-for-zulip · 2019-12-17T02:24:11Z

The new version is rather larger, as it contains more tests than were originally thought viable... and this GitHub PR is already approaching the limit of usefulness. Closing, to be reopened with a fresh slate and a pointer.

rk-for-zulip requested a review from gnprice November 26, 2019 19:05

rk-for-zulip force-pushed the presence-heartbeat branch from 06ea013 to 23c8f0d Compare November 26, 2019 22:36

rk-for-zulip marked this pull request as ready for review November 26, 2019 22:39

rk-for-zulip force-pushed the presence-heartbeat branch from 23c8f0d to 4e2dd81 Compare November 26, 2019 22:42

gnprice reviewed Dec 3, 2019

View reviewed changes

rk-for-zulip added 8 commits December 5, 2019 13:31

api [nfc]: rename reportPresence parameter

93a82b8

Mobile apps are not usually described as "having focus", and the underlying parameter value is `"active"` anyway.

presence: add Heartbeat class (+ tests)

9968851

Add `Heartbeat`, a generic-looking class which will be used to perform presence-reporting. Also add tests, to ensure that it behaves as expected.

deps: add Lolex

b2ace42

Jest doesn't currently (v24.9.0) mock out `Date.now()` when using fake timers. This is expected to change in 25.0 when Jest switches over to using Lolex for its fake timer implementation. We need it today, though, so we'll jump the gun on that.

heartbeat: change presence-reporting protocol

fb80a41

Following discussion on chat.zulip.org's #mobile-team channel, adjust the presence-reporting protocol so that it never sends "idle". https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/presence.20and.20notifications

heartbeat [nfc]: remove state parameter from callback

f013a72

The heartbeat callback is now only ever called with `true`. Simplify the associated code by removing the now-constant argument.

rk-for-zulip force-pushed the presence-heartbeat branch from 4e2dd81 to f013a72 Compare December 6, 2019 01:22

rk-for-zulip requested a review from gnprice December 9, 2019 17:51

gnprice reviewed Dec 9, 2019

View reviewed changes

This was referenced Dec 9, 2019

Presence-reporting logic is confused, in overlapping ways #3699

Closed

Document the data model for user presence zulip/zulip#13519

Open

rk-for-zulip closed this Dec 17, 2019

rk-for-zulip mentioned this pull request Dec 17, 2019

Presence heartbeat (v2) #3727

Merged

gnprice added the a-presence/status Presence, and "away status" label Mar 13, 2020

rk-for-zulip mentioned this pull request Mar 19, 2020

Break up handlers for native events #3982

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Presence: Heartbeat #3704

Presence: Heartbeat #3704

rk-for-zulip commented Nov 26, 2019 •

edited by gnprice

Loading

rk-for-zulip commented Dec 2, 2019

gnprice left a comment

gnprice Dec 3, 2019

rk-for-zulip Dec 4, 2019

gnprice Dec 3, 2019

rk-for-zulip Dec 4, 2019

gnprice Dec 4, 2019

rk-for-zulip Dec 5, 2019

gnprice Dec 5, 2019

gnprice Dec 9, 2019

rk-for-zulip Dec 16, 2019 •

edited

Loading

rk-for-zulip commented Dec 4, 2019

gnprice commented Dec 4, 2019

rk-for-zulip commented Dec 6, 2019

gnprice left a comment

gnprice Dec 9, 2019

gnprice Dec 9, 2019

gnprice Dec 9, 2019

rk-for-zulip Dec 16, 2019

gnprice Dec 9, 2019

gnprice Dec 9, 2019

gnprice Dec 9, 2019

gnprice Dec 9, 2019

gnprice Dec 9, 2019

rk-for-zulip Dec 17, 2019

gnprice Dec 9, 2019

rk-for-zulip Dec 10, 2019

gnprice Dec 9, 2019

rk-for-zulip Dec 10, 2019

gnprice Dec 9, 2019

rk-for-zulip Dec 10, 2019 •

edited

Loading

rk-for-zulip Dec 10, 2019 •

edited

Loading

gnprice commented Dec 9, 2019

rk-for-zulip commented Dec 17, 2019

Presence: Heartbeat #3704

Presence: Heartbeat #3704

Conversation

rk-for-zulip commented Nov 26, 2019 • edited by gnprice Loading

rk-for-zulip commented Dec 2, 2019

gnprice 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

Choose a reason for hiding this comment

rk-for-zulip Dec 16, 2019 • edited Loading

Choose a reason for hiding this comment

rk-for-zulip commented Dec 4, 2019

gnprice commented Dec 4, 2019

rk-for-zulip commented Dec 6, 2019

gnprice 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

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

rk-for-zulip Dec 10, 2019 • edited Loading

Choose a reason for hiding this comment

rk-for-zulip Dec 10, 2019 • edited Loading

Choose a reason for hiding this comment

gnprice commented Dec 9, 2019

rk-for-zulip commented Dec 17, 2019

rk-for-zulip commented Nov 26, 2019 •

edited by gnprice

Loading

rk-for-zulip Dec 16, 2019 •

edited

Loading

rk-for-zulip Dec 10, 2019 •

edited

Loading

rk-for-zulip Dec 10, 2019 •

edited

Loading