DEV: Various behind-the-scenes improvements to PresenceChannel (PR #14518)

  • Allow the /presence/get endpoint to return multiple channels in a single request (limited to 50)
  • When multiple presence channels are initialized in a single Ember runloop, batch them into a single GET request
  • Introduce the presence-pretender to allow easy testing of PresenceChannel-related features
  • Introduce a use_cache boolean (default true) on the the server-side PresenceChannel initializer. Useful during testing.

GitHub

I know you’ve done some investigation into the safety of this, but I can’t help but think it’s still slightly risky. We should just keep an eye on test runs after this in case we notice them failing more often or taking too long.

    let result;
    try {
      result = await this._initialDataAjax;
    } catch (e) {
      later(this, this._makeInitialDataRequest, PRESENCE_GET_RETRY_MS);
      throw e;
    } finally {
      this._initialDataAjax = null;
    }

Minor but we can use a finally block here.

Just curious, do we need to clear channels after each test?

I think we usually try to avoid magic numbers and preferably use a constant with a name that reflects what the number represents.

I have a small concern here, channel.can_view? triggers a DB query if the channel is configured with allowed_group_ids. The worst case here is that this endpoint will trigger 50 DB query which I think is not ideal. Perhaps we need a way to bulk check can_view? for a bunch of channels against a single user?

Not very sure here but I’m wondering why we need to return the channel name if the user is not allowed to view it.

Minor note about the assertions above in this tests. I feel like we can merge all of this into a single request since we now accept multiple channel names. The response also makes it easy for us to test each channel individually so we don’t really need multiple requests here.

Do we want to retry unconditionally here? Are there only a few specific errors we want to retry on?

Aside from some comments the changes in general look OK to me.

Yes I think we do. If this fails for any reason, the application will end up in a broken state until a refresh. Better to keep retrying every 5 seconds until the problem is resolved (e.g. the network connection is restored)

🤦 Yes we do. I have this locally, but didn’t include it in the PR.

The idea is that "/channel-name": null means “this channel does not exist”. The client-side interprets this, and throws a PresenceChannelNotFound error.

Previously “does not exist” was indicated by a 404 error, but that’s no longer possible because multiple channels are returned in a single API call.

Fixed in 127c9042

Done in a099e38b0c308f203fab0616dde66c3ebf727785

I think accepting a lambda is unnecessary here. We can just compute the user_group_ids from the method caller and pass it in to the method. user_group_ids ||= GroupUser.where(user_id: user_id).pluck("group_id") should work I think.

The aim was to avoid fetching the group_ids unless they were actually needed. (if all requested channels are public, or user-id secured, then there is no need).

But you’re right that the lambda is a messy API. Maybe we just accept the cost of fetching groups every time.