MSC2753: Peeking via sync (take 2)

[ara4n]

Rendered

A first cut at an alternative solution to peeking via /sync, using a /peek method rather than messing around with filters.

Hopefully obsoletes #1776. Credit to @Half-Shot for suggesting this approach at #1776 (review).

[bwindels]

Overall, this looks like a nice API to work with as a client developer. Have left some comments for the specific use cases mentioned though, which each seem to have their own intuitive scope and lifetime for a peek.

[turt2live]

generally seems in the right direction - just a few comments in addition to the other ongoing threads.

Please also line wrap at ~100 characters.

[erikjohnston]

This seems relatively sane, modulo concerns with clients accidentally not cleaning up after themselves. (Plus to what extent this interacts with the request format for paginated sync).

I wholeheartedly approve of putting these rooms in a dedicated peek section in /sync response

[carlbordum]

Hi, we are building something slightly-novel on matrix. Just wanted to let you know that this MSC would be an improvement for us 👍

[richvdh]

Thoughts on this particularly welcome. (I in no way promise to follow the result, but feel free to express opinion with a reaction on this comment: 👍 for "require an ID", 👎 for "allow either ID or alias")

[Half-Shot]

require an ID for me. If a client is trying to peek an alias, they will have likely already (or should have) called the directory API to confirm it's existence, IMO.

[ara4n]

we want peeks to be as fast as possible, and adding in a roundtrip to first lookup the alias is completely unnecessary and so adds latency for no good reason. So i reckon we should keep it supporting aliases.

[richvdh]

(a) if the peek is over federation, there are going to be multiple requests anyway, and it's going to be slow anyway, and having two API calls gives better feedback to the client as to which part failed. (b) As H-S says, in most cases we'll know the ID anyway (in particular #1772 uses IDs everywhere).

I'm about 80% in favour of requiring an ID but can see both arguments.

[ara4n]

expecting clients to maintain a clock and be NTP synced feels unnecessary. it's also inconsistent with the renewal_interval approach that #2444 takes. can't we just return a lifetime for the peek?

[neilalexander]

(Don't we already expect clients to maintain a clock to keep TLS alive?)

[richvdh]

can't we just return a lifetime for the peek?

yes we can, and I was in somewhat two minds about it. renewal_interval? ttl for consistency with /voip/turnServer ? lifetime for consistency with m.call.invite ? <something>_ms for consistency with retry_after_ms ?

[richvdh]

(Don't we already expect clients to maintain a clock to keep TLS alive?)

I'm not following this. Most client impls have nothing to do with TLS.

[neilalexander]

I'm not following this. Most client impls have nothing to do with TLS.

I mean that if the client's clock is that far out of sync, that they aren't going to be able to set up a TLS connection to the CS API anyway.

[richvdh]

oh, I see. Possibly, yes.

[ara4n]

so can we make it a renewal_interval for consistency with #2444 (and to avoid people having to be NTP synced?)

[richvdh]

well I agree that it may as well be consistent with #2444, but we can change #2444 to match whatever we decide here.

renewal_interval implies a fixed interval, whereas the server might want to vary the lifetime depending on what else is going on. I'd prefer lifetime.

OTOH I don't feel that strongly and don't want to bikeshed it, so if you are keen on renewal_interval we can go with that.

[ara4n]

It feels really weird to lose the room id or alias from the URL - we won't be able to see it in logs, and given it's the main object of the method, it feels like it should be in the URL rather than buried as a field in the POST request body.

I'm happy to dump the servers into the post body (rather than being querystring params) if it makes folks feel happier though.

Otherwise this is looking good to me.

[t3chguy]

Agreed, why can't this use the same existing shape as /_matrix/client/r0/join/{roomIdOrAlias}?server_name=x&server_name=y

[richvdh]

I'll stick the room ID/alias back in the URI. As for the server_name, we went round this a few times in a chat room, but I'll try to summarise:

• I think consistency with /join is a bit of a false idol - the semantics are necessarily different, we have to add extra fields to the response, and currently (for better or worse) the body of /join ends up in the content of the join event (cf synapse secretly uses body of /join request as the join event content synapse#8763), which obviously we have no equivalent for here.
• Unnecessarily mixing query-params and body params seems very weird, and something that I'd like to fix in /join (https://github.com/matrix-org/matrix-doc/issues/2864). I'd rather not expand that problem to another API.

[t3chguy]

and for things like matrix-static which do not want to maintain /sync and actively peek over a large set of rooms.

[t3chguy]

surely not given that it would still be per-device and those clients wouldn't be calling the new /peek API

[Dominaezzz]

Typo

Suggested change

	API closer to `/join`. On the other hand, they probabably aren't actually
	API closer to `/join`. On the other hand, they probably aren't actually

[Dominaezzz]

Pointing out some issues with the /sync side of things.

A potential solution could be to provide some sort of "continuation token" in the leave section, when peeking ends.
When a client does /join or /peek again, they also pass this optional "continuation token", then the server may send a delta instead of the full state of the room.
The server may decide to ignore the token if it's too old or it is too expensive to calculate the delta.
This could also make it easier for clients to manage their peeks. Instead of keeping track of time, they can just pass this token to continue peeking.

This continuation token would be an awesome enhancement to the /join and /leave sync state management too.

[Dominaezzz]

If someone /peeks, they'll see the room in the peek section and eventually the leave section.
If someone /peeks, then eventually joins the room, it will further appear in the join section.
Now, what if we have these sequence of events.

• Device A /peeks into a room.
• Device A /syncs once and gets the (initial) state of the peeked room.
• Device B /joins the room that Device A is peeking.
• Device B leaves the room almost immediately.
• Device A /syncs again ... and gets what?

According to this paragraph, it's reasonable to infer that Device A will see the room appear in the leave section, since the user has left the room but this client has some state that needs updating.

Does this mean my (Device A) device specific /peek has been cancelled by this other random device?
Sure, I could /peek again but room state can be expensive to get. I already have fairly fresh state locally that just needs an incremental update.

[kevincox]

The other question is what is the desired UX. I would expect that if I was peeking a room in the context of considering joining I would expect that joining on another device transitioned the view into the room itself as a member. In that case leaving the room should bring me to whatever the "left room" experience is.

However there are other cases for peeking such as spaces or "X as rooms" (such as "profiles as rooms") where you would not want to be unpeeked based on joining the space. However this likely applies for both this device and another device joining the space.

It seems that the best option is the the room is left in the peek list if it is left, whether on this client or another one. Then this client can decide if it still needs that peek (for example it is peeking the space for membership info) or if it is no longer necessary (for example it was for a room preview, and after joining and leaving the user should now be in a "left room" UX where no new info is required).

[Dominaezzz]

If I'm peeking into a room, then someone invites me to it, then rescinds the invitation, has my peek been cancelled?
How do I differentiate between my invite being rescinded and my peek ending?

Also, if my peek ended, I have to start again, etc. More on this in the previous comment.

[kevincox]

Suggested change

	The server ratelimit requests to `/peek` and returns a 429 error with
	The server should ratelimit requests to `/peek` and returns a 429 error with

[kevincox]

Suggested change

	a successful response is returned with the updated `peek_expiry_ts`.
	a successful response is returned with an updated `peek_expiry_ts`.

"an" seems to imply that the client is providing the expiry which is not the case here.

[kevincox]

It would be useful to have a specific error code here. Especially if an alias is accepted as the client may not be able that it is joined to the room already. In particular it seems that at least the resolved room ID should be returned.

[kevincox]

This leads to an unhappy situation where a delayed request may fail because the peek has expired. IIUC this would be hard for for the client to handle gracefully in any other way then just assuming success. Since this is the case why do we differentiate here? Why not make this return success in any case where the client is not peeking when the response is returned. AKA why not make this idempotent.

[kevincox]

The other question is what is the desired UX. I would expect that if I was peeking a room in the context of considering joining I would expect that joining on another device transitioned the view into the room itself as a member. In that case leaving the room should bring me to whatever the "left room" experience is.

However there are other cases for peeking such as spaces or "X as rooms" (such as "profiles as rooms") where you would not want to be unpeeked based on joining the space. However this likely applies for both this device and another device joining the space.

It seems that the best option is the the room is left in the peek list if it is left, whether on this client or another one. Then this client can decide if it still needs that peek (for example it is peeking the space for membership info) or if it is no longer necessary (for example it was for a room preview, and after joining and leaving the user should now be in a "left room" UX where no new info is required).

[kevincox]

The client cost to "forgetting" a peek is very low. However the server may need to do some work (if /sync is poll-based for all rooms). I think the only client cost is if you are offline for a while and get a huge number of events before you get the chance to unpeek a room you no longer need.

One option would be to return all peeked rooms in the sync response even if they have no events. The upside is that you constantly (or periodically) remind the clients what they are peeking but the downside is some extra transfer and parsing (although this can be made arbitrarily low by only sending a "reminder" for each room occasionally).

[ShadowJonathan]

Could a rule be added where a user can peek the room if they have been invited to that room? (If history_visibility is shared or invited)

This would allow "previewing the room before joining it", i.e. "do you want to join this room" with some context.

This would also allow clients to implement "accept/reject DM request", and display DMs in a similar manner as they do in many IM clients today; by adding the DM to the DM list as any other DM.

Such a rule could probably synergize with MSC2199 and MSC2444, providing users more context about the rooms/DMs they're invited to, and be able to accept/reject with more information at hand.

[Kladki]

It is mentioned, but it would be better to explicitly state under what conditions a user can peek into a given room.

[ara4n]

Something this should also handle is supporting /event and /context into peeked rooms.

[ShadowJonathan]

Shouldn't that be accepted/handled automatically as long as a peek is active?