Presence heartbeat (v2) #3727

rk-for-zulip · 2019-12-17T02:30:57Z

Rewrite, and possibly unnecessarily thoroughly test, the presence-heartbeat code.

Resumption of PR #3704.

rk-for-zulip · 2019-12-17T19:45:04Z

The linter failure in the previous CI failure has been addressed by linter pragmata for now. (I thought I'd used that construct previously; but no, that was inside a .forEach.)

The current CI failure is due to a tests-only auxiliary file being (not unreasonably) mistaken for an actual test file by Jest, because it's in a subdirectory of the __tests__ folder that it's being used from. It clearly needs to be moved, but I see nowhere appropriate for it at present, and have no strong opinion about where it should go. A dev-src/ directory at the root, perhaps?

rk-for-zulip · 2019-12-18T19:14:20Z

(Addendum: I haven't simply done that because, on reflection, the additional tests they're in support of may be more code than is worth having around. I'd prefer a second opinion on that.)

chrisbobbe · 2020-01-15T22:19:02Z

I'm not sure I can speak to the necessity of those tests, but maybe for the Jest failure -- it's a bit of a patch, but perhaps the code in src/presence/__tests__/aux/interval-set.js doesn't need to be in a separate file; it's not very long, it's only imported by one file, heartbeat-test.js, and that file includes similar helper code (the Lolex class) that hasn't been deemed large enough to separate.

Some brief research hasn't revealed any recommendations from Jest on where to put such things.

rk-for-zulip · 2020-01-15T23:26:22Z

[...] and that file includes similar helper code (the Lolex class) that hasn't been deemed large enough to separate.

It probably should be, though: there are other timer-dependent tests (see: rg 'Date\.now' -g'**/__tests__/**') which could, and probably should, make use of Lolex.

They may also want to make use of interval trees – in which case I'll have to properly separate out... well, a lot of things – but even then the interval-tree library wrapper should probably be a sub-library, just for the sake of separation of concerns.

(If I just wanted to silence the Jest failure, I could put a dummy test in that file.)

rk-for-zulip · 2020-01-30T22:18:20Z

Pinging @gnprice.

gnprice · 2020-02-27T01:21:10Z

Marking priority, following the issue.

gnprice · 2020-02-27T01:43:09Z

Thanks again @ray-kraesig for these changes! Now that the key parts of #3886 are in, want to rebase this to take advantage of Lolex being in master? (And the remaining commits of #3886 should be landing shortly, too, but I think those won't interact with this branch.)

I think the now-merged commits from #3886 also resolve the Jest file-layout questions discussed above. But please mention if I've missed something and that's not the case.

I'm also about to go read through this code -- I'll just skip over for now the bits that look like they were covered by #3886.

gnprice

And read through the first half of this! Namely these commits, corresponding to what was in #3704:

37c5bb7 api [nfc]: rename reportPresence parameter
e12c452 presence: strip throttling from reportPresence wrapper
a32a1ac presence: add Heartbeat class (+ tests)
987a9b7 presence: move all presence-reporting logic into new HeartbeatComponent
f17b7d6 heartbeat: convert tests to use Lolex
e0a265d heartbeat: change presence-reporting protocol
3ca8fe1 heartbeat [nfc]: remove state parameter from callback

Thanks for the revisions! I like the changes. This looks close; there are a couple of things below.

I haven't yet read in detail the subsequent changes:

0d590a5 heartbeat [nfc]: add fine-grained event tracking to tests
5491e5e heartbeat [nfc]: extend heartbeat-test event system
d612271 deps: add interval-set library
27aa07c heartbeat: define additional constraints
0d14548 heartbeat tests: add app-suspension test cases

I think it might be simplest to reserve those for a subsequent PR.

One small commit-level remark: the summary line "heartbeat: define additional constraints" sounds to me like it's going to change something in the code's behavior. (There are a lot of things "constraints" could mean.) It turns out it doesn't, adding only a comment and some tests, and it'd be great to communicate that in the summary line. Perhaps s/define/Document, and test,/?

gnprice · 2020-02-27T02:09:36Z

src/users/usersActions.js

-  const now = new Date();
-  if (differenceInSeconds(now, lastReportPresence) < 60) {
-    // TODO throttle properly; probably fold setInterval logic in here


Commit-level comments on this commit:
e12c452 presence: strip throttling from reportPresence wrapper

Nit: "would fix #3699 as literally reported" is no longer quite right, as we've edited #3699 so it describes the problem more broadly.

Related nit: "would fix #..." is a dangerous phrase to put in a commit message, because GitHub sees the "fix" and doesn't understand the "would". :-) Should basically always get reworded to hide from GitHub's auto-close rules, with e.g. a circumlocution like "would be a fix for #...".

Stepping back, though, I think my comment on this commit from the previous PR still applies:
#3704 (review)

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.

Stepping back, though, I think my comment on this commit from the previous PR still applies:
#3704 (review)

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.

The fix for this is basically to squash the second commit into the fourth commit. As there's not much substance to it, done. (Although this does make the commit message uncomfortably crowded. 😕)

I like the result, thanks!

The commit message does get a bit longer. I think it might actually on net be simpler than before, though. One way it's certainly simpler is that the "If the user logs in and out" bullet previously had to tell a multi-step story ("This was previously concealed by ... which was itself removed in a previous patch") and that's now a simple before/after.

gnprice · 2020-02-27T02:12:21Z

src/presence/PresenceHeartbeat.js

+    AppState.addEventListener('change', this.updateHeartbeatState);
+  }
+
+  componentWillUnmount() {
+    AppState.removeEventListener('change', this.updateHeartbeatState);


We had a comment thread on this bit over on the previous PR: #3704 (comment)

Your explanation there makes sense to me! I'm convinced.

I also wanted to mention that thread here because you'd said:

(On the other hand, I'd argue that the lone existing use of .isActive, over in src/events/eventMiddleware.js, is a bug! Or at least is closely involved in one. But that's not directly relevant here; I'll elaborate somewhere else.)

and I don't want that thought to get lost. 🙂 Want to elaborate on that -- perhaps in a new issue, or a chat thread?

Eh, I might as well express my current thoughts on that in the form of commit messages.

Opened PR #3936.

gnprice · 2020-02-27T02:27:59Z

src/presence/PresenceHeartbeat.js

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


On the previous PR we discussed a previous version of this comment: #3704 (comment) Thanks for the updates!

I think it's pretty reasonable to say that the term "pure" here is confusing and of questionable veracity.

I think what my thought here boils down to now is that that complaint is really all about the naming of PureComponent. What we're doing is 100% consistent with the expectations of a React PureComponent, which are documented here -- it's just that those expectations don't include all the things that you might (reasonably!) expect the word "pure" to mean, or to be true of "a pure component".

A comment about that mismatch of PureComponent vs. the meaning of "pure" in general would be fine to have here. (Certainly it wouldn't be the first comment in our codebase that complains about a design choice that happened somewhere else 😉 -- I've written plenty of those.) What I don't want is a warning that we're hacking around or breaking some API expectation that potentially could expose us to problems when the React implementation changes, in a spot where in fact we aren't.

What I don't want is a warning that we're hacking around or breaking some API expectation that potentially could expose us to problems when the React implementation changes, in a spot where in fact we aren't.

Ah, I see the ambiguity now. Amended significantly.

New version looks great!

rk-for-zulip · 2020-02-29T00:13:37Z

I think it might be simplest to reserve those for a subsequent PR.

Fair enough! Snipped for now.

gnprice · 2020-03-04T22:33:24Z

Thanks @ray-kraesig for the revisions!

One question and a nit, both in the commit messages.

Fixes (the original version of) #3699, and is work towards #3659.

Seems to me like it actually fixes #3699. Is there a particular aspect of that that's still open that you have in mind?

Maybe this is a sign that my revised title was too vague 🙂 : "Presence-reporting logic is confused, in overlapping ways". I think the bar for closing the issue isn't that we become sure there are no bugs or confusions in the new version -- just that it's no longer confused in the ways we knew about. And of course a related good thing to achieve (as this PR does) is that it's written in a way that we can hope is less prone to confusion. 😉

heartbeat: change presence-reporting protocol

Following discussion on chat.zulip.org's #mobile-team channel, adjust

s/channel/stream/ :smile:

Oh, and I guess a second nit: s/heartbeat/presence/ in that summary line would feel a bit more on-point to me. I think it's because this change is mainly aimed at the server-facing reporting of presence, rather than the internal implementation choices (even though it involves making changes to the latter to accomplish its changes to the former.)

rk-for-zulip · 2020-03-05T01:09:32Z

Seems to me like it actually fixes #3699. Is there a particular aspect of that that's still open that you have in mind?

As of that commit, yes: idle messages are still sent, under the (existing) confusion that they should be. The following commit should have the relevant fix-note. (Done.)

gnprice · 2020-03-06T02:55:03Z

As of that commit, yes: idle messages are still sent, under the (existing) confusion that they should be. The following commit should have the relevant fix-note. (Done.)

Ah, got it. Thanks!

I'll merge shortly. I think I'll make one small further edit to the commit messages: on this "Fixes (the original version of) #3699" commit, I'll add a mention that a fix to the remainder of that issue is coming up next. Reduces the suspense a bit for the reader. 😉

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

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

Replace the existing presence-reporting logic with HeartbeatComponent, instantiated as part of a new zero-display React component located under AppEventHandlers. In particular, strip the throttling logic from `reportPresence`. This fixes at least three issues: * If the user logged in and out, or otherwise performed an additional "initial fetch" without killing the app, the presence timer would be started multiple times. (The negative effects of this were previously concealed by the throttling logic.) * `idle` events were not treated differently from `active` events by the throttling logic, and would almost invariably be swallowed. * If the user went idle (e.g. by hitting the Home button), the presence timer would still run, and would still attempt to send `active` notifications. (These attempts were often suppressed while the app was in the background, but this was never guaranteed.) Fixes the original version of zulip#3699; the next commit will complete the fix for that issue. This is also work towards zulip#3659. Unfortunately, there is no user-visible change yet: modifications will be needed on the Zulip server side as well.

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

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

rk-for-zulip requested a review from gnprice December 17, 2019 02:30

rk-for-zulip force-pushed the presence-heartbeat branch from e590d66 to 0d14548 Compare December 17, 2019 18:27

chrisbobbe mentioned this pull request Jan 15, 2020

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

Closed

rk-for-zulip mentioned this pull request Jan 30, 2020

makeBackoffMachine: Improve backoff sleep logic by making it local. #3841

Merged

rk-for-zulip mentioned this pull request Feb 10, 2020

Add Lolex for tests involving time #3886

Merged

gnprice added the P1 high-priority label Feb 27, 2020

gnprice reviewed Feb 27, 2020

View reviewed changes

rk-for-zulip force-pushed the presence-heartbeat branch 2 times, most recently from e373c09 to 190bc38 Compare February 27, 2020 21:55

rk-for-zulip mentioned this pull request Feb 29, 2020

Remove isActive #3936

Merged

rk-for-zulip force-pushed the presence-heartbeat branch from 190bc38 to 6600f12 Compare February 29, 2020 00:12

rk-for-zulip force-pushed the presence-heartbeat branch from 6600f12 to 3675c24 Compare March 5, 2020 01:07

rk-for-zulip added 5 commits March 5, 2020 18:55

api [nfc]: rename reportPresence parameter

cfcb01b

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

presence: add Heartbeat class (+ tests)

ff65ced

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

heartbeat [nfc]: remove state parameter from callback

9e3d52d

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

gnprice force-pushed the presence-heartbeat branch from 3675c24 to 9e3d52d Compare March 6, 2020 03:08

gnprice merged commit 9e3d52d into zulip:master Mar 6, 2020

gnprice mentioned this pull request Mar 6, 2020

Never get notifications (when recently used webapp, or app is in background) #3659

Open

rk-for-zulip deleted the presence-heartbeat branch March 6, 2020 18:37

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

chrisbobbe mentioned this pull request Apr 3, 2020

Move exampleData.js into auxiliary #3990

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Presence heartbeat (v2) #3727

Presence heartbeat (v2) #3727

rk-for-zulip commented Dec 17, 2019 •

edited

Loading

rk-for-zulip commented Dec 17, 2019

rk-for-zulip commented Dec 18, 2019

chrisbobbe commented Jan 15, 2020 •

edited

Loading

rk-for-zulip commented Jan 15, 2020

rk-for-zulip commented Jan 30, 2020

gnprice commented Feb 27, 2020

gnprice commented Feb 27, 2020

gnprice left a comment

gnprice Feb 27, 2020

rk-for-zulip Feb 29, 2020

gnprice Mar 4, 2020

gnprice Feb 27, 2020

rk-for-zulip Feb 29, 2020

gnprice Feb 27, 2020 •

edited

Loading

rk-for-zulip Feb 29, 2020

gnprice Mar 4, 2020

rk-for-zulip commented Feb 29, 2020

gnprice commented Mar 4, 2020

rk-for-zulip commented Mar 5, 2020

gnprice commented Mar 6, 2020

Presence heartbeat (v2) #3727

Presence heartbeat (v2) #3727

Conversation

rk-for-zulip commented Dec 17, 2019 • edited Loading

rk-for-zulip commented Dec 17, 2019

rk-for-zulip commented Dec 18, 2019

chrisbobbe commented Jan 15, 2020 • edited Loading

rk-for-zulip commented Jan 15, 2020

rk-for-zulip commented Jan 30, 2020

gnprice commented Feb 27, 2020

gnprice commented Feb 27, 2020

gnprice left a comment

Choose a reason for hiding this comment

gnprice Feb 27, 2020

Choose a reason for hiding this comment

rk-for-zulip Feb 29, 2020

Choose a reason for hiding this comment

gnprice Mar 4, 2020

Choose a reason for hiding this comment

gnprice Feb 27, 2020

Choose a reason for hiding this comment

rk-for-zulip Feb 29, 2020

Choose a reason for hiding this comment

gnprice Feb 27, 2020 • edited Loading

Choose a reason for hiding this comment

rk-for-zulip Feb 29, 2020

Choose a reason for hiding this comment

gnprice Mar 4, 2020

Choose a reason for hiding this comment

rk-for-zulip commented Feb 29, 2020

gnprice commented Mar 4, 2020

rk-for-zulip commented Mar 5, 2020

gnprice commented Mar 6, 2020

rk-for-zulip commented Dec 17, 2019 •

edited

Loading

chrisbobbe commented Jan 15, 2020 •

edited

Loading

gnprice Feb 27, 2020 •

edited

Loading