Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Timeline needs to refresh when we see a MSC2716 marker event #2299

Conversation

MadLittleMods
Copy link
Contributor

@MadLittleMods MadLittleMods commented Apr 14, 2022

Inform the client that historical messages were imported in the timeline and they should refresh the timeline in order to see the new events.

Companion matrix-react-sdk PR: matrix-org/matrix-react-sdk#8354

The marker events are being used as state now because this way they can't be lost in a timeline gap. Regardless of when they were sent, we will still have the latest version of the state to compare against. Any time we see our latest state value change for marker events, prompt the user that the timeline needs to refresh.

In a sync meeting with @ara4n, we came up with the idea to make the marker events as state events. When the client sees that the m.room.marker state changed to a different event ID, it can throw away all of the timeline and re-fetch as needed.

For homeservers where the same problem can happen, we probably don't want to throw away the whole timeline but it can go up the unsigned.replaces_state chain of the m.room.marker state events to get them all.

In terms of state performance, there could be thousands of marker events in a room but it's no different than room members joining and leaving over and over like an IRC room.

-- matrix-org/matrix-spec-proposals#2716 (comment)

Why are we just setting timlineNeedsRefresh (and prompting the user) instead of automatically refreshing the timeline for the user?

If we refreshed the timeline automatically, someone could cause your Element client to constantly refresh the timeline by just sending marker events over and over. Granted, you probably want to leave a room like this 🤷. Perhaps also some sort of DOS vector since everyone will be refreshing and hitting the server at the exact same time.

In order to avoid the timeline maybe going blank during the refresh, we could re-fetch the new events first, then replace the timeline. But the points above still stand on why we shouldn't.

Todo

Dev notes

yarn jest spec/integ/matrix-client-syncing.spec.js --silent=false --verbose=false

        Error.stackTraceLimit = Infinity;
        console.log(`setStateEvents timelineWasEmpty=${markerFoundOptions && markerFoundOptions.timelineWasEmpty} stateEvents:\n`, stateEvents.map((ev) => {
            return `\t${ev.getType()} ${ev.getId()}`;
        }).join('\n'), new Error().stack);

Functions with overloads:

eventTimelineset.addLiveEvent
eventTimelineSet.addEventToTimeline
eventTimeline.addEvent
room.addLiveEvent
room.addLiveEvents

room.resetLiveTimeline(null, null);
addEventsToTimeline
addEventToTimeline
addLiveEvent

resetLiveTimeline

resetAllTimelines

removeFilteredTimelineSet


syncFromCache
processSyncResponse
processRoomEvents
room.addLiveEvents


sessionStore


processSyncResponse
processRoomEvents
initialiseState
setStateEvents

load
getEventTimeline
initialiseState
setStateEvents
hasSyncedBefore
getSyncState()
function serializeDebugInfoFromTimelineSets(timelineSets): { [key: string]: string[] }[] {
    const serializedEventIdsInTimelineSet = timelineSets.map((timelineSet) => {
        const timelineMap = {};

        const timelines = timelineSet.getTimelines();
        const liveTimeline = timelineSet.getLiveTimeline();

        timelines.forEach((timeline, index) => {
            const forwardPaginationToken = timeline.getPaginationToken(EventTimeline.FORWARDS);
            const backwardPaginationToken = timeline.getPaginationToken(EventTimeline.BACKWARDS);

            // Add a special label when it is the live timeline so we can tell
            // it apart from the others
            const isLiveTimeline = timeline === liveTimeline;
            const paginationString = `--f=${forwardPaginationToken}--b=${backwardPaginationToken}`;
            timelineMap[isLiveTimeline ? `${index}-${timeline.toString()}--liveTimeline${paginationString}` : `${index}-${timeline.toString()}${paginationString}`] = timeline.getEvents().map(ev => ev.getId());
        });

        return timelineMap;
    });

    return serializedEventIdsInTimelineSet;
}

console.log('refresh timeline serializeDebugInfoFromTimelineSets', serializeDebugInfoFromTimelineSets(this.timelineSets));

Here's what your changelog entry will look like:

✨ Features

> In a [sync meeting with @ara4n](https://docs.google.com/document/d/1KCEmpnGr4J-I8EeaVQ8QJZKBDu53ViI7V62y5BzfXr0/edit#bookmark=id.67nio1ka8znc), we came up with the idea to make the `marker` events as state events. When the client sees that the `m.room.marker` state changed to a different event ID, it can throw away all of the timeline and re-fetch as needed.
>
> For homeservers where the [same problem](matrix-org/matrix-spec-proposals#2716 (comment)) can happen, we probably don't want to throw away the whole timeline but it can go up the `unsigned.replaces_state` chain of the `m.room.marker` state events to get them all.
>
> In terms of state performance, there could be thousands of `marker` events in a room but it's no different than room members joining and leaving over and over like an IRC room.
>
> *-- matrix-org/matrix-spec-proposals#2716 (comment)
Comment on lines +327 to +338
// It would be nice if we could also specifically tell whether the
// historical messages actually affected the locally cached client
// timeline or not. The problem is we can't see the prev_events of
// the base insertion event that the marker was pointing to because
// prev_events aren't available in the client API's. In most cases,
// the history won't be in people's locally cached timelines in the
// client, so we don't need to bother everyone about refreshing
// their timeline. This works for a v1 though and there are use
// cases like initially bootstrapping your bridged room where people
// are likely to encounter the historical messages affecting their
// current timeline (think someone signing up for Beeper and
// importing their Whatsapp history).
Copy link
Contributor Author

@MadLittleMods MadLittleMods Apr 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideas welcome ^

In my initial thoughts, I wasn't too keen on adding an extra field to the marker event to say where the history was inserted next to.

I'm not too keen to add an extra field to the marker or insertion event indicating where the history is imported next to as there isn't a way to enforce that it's true (a homeserver could lie and make it up). I'd rather keep the source of truth in the prev_events.

-- #2282 (comment)

But I don't think I'm leaning as far away now since a homeserver could also make up the m.insertion_id field that we have now. We could add a m.insertion_prev_events field.

{
    "type": "m.room.marker",
    "sender": "@appservice:example.org",
    "content": {
        "m.insertion_id": insertion_event.event_id,
        "m.insertion_prev_events": insertion_event.prev_events
    },
    "room_id": "!jEsUZKDJdhlrceRyVU:example.org",
    "origin_server_ts": 1626914158639,
}

We can also iterate this in another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also iterate this in another PR.

Opting for this option ⏩. This is a nice to have.

@MadLittleMods MadLittleMods changed the title Draft: Timeline needs to refresh when we see a MSC2716 marker event Timeline needs to refresh when we see a MSC2716 marker event Apr 14, 2022
@MadLittleMods MadLittleMods marked this pull request as ready for review April 14, 2022 06:53
@MadLittleMods MadLittleMods requested a review from a team as a code owner April 14, 2022 06:53
src/@types/event.ts Outdated Show resolved Hide resolved
src/models/event-timeline-set.ts Outdated Show resolved Hide resolved
src/models/room.ts Outdated Show resolved Hide resolved
@MadLittleMods MadLittleMods requested a review from t3chguy May 28, 2022 06:41
@matrix-org matrix-org deleted a comment from codecov-commenter May 30, 2022
Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is getting very close now

See #2299 (comment)

 - Move the preferred overload to the top so it's suggested first
 - Add `@deprecated` decorators to the deprecated overload
 - Add deprecation log warning when we see usage of the deprecated function
@MadLittleMods MadLittleMods requested a review from t3chguy June 1, 2022 08:50
Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit: thanks for perservering with this!

@@ -637,7 +1109,7 @@ describe("MatrixClient syncing", function() {
awaitSyncEvent(),
]).then(function() {
const room = client.getRoom(roomTwo);
expect(room).toBeDefined();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference, I updated this because room can be null which this didn't catch before (only caught undefined)

src/client.ts Outdated Show resolved Hide resolved
spec/unit/room-state.spec.js Outdated Show resolved Hide resolved
spec/integ/matrix-client-event-timeline.spec.js Outdated Show resolved Hide resolved
src/client.ts Outdated Show resolved Hide resolved
Previously copy-paste from `client.getEventTimeline()`
Previously copy-paste from `client.getEventTimeline()`
@MadLittleMods MadLittleMods merged commit b64dbdc into develop Jun 1, 2022
@MadLittleMods MadLittleMods deleted the madlittlemods/refresh-timeline-when-we-see-msc2716-marker-events-v2 branch June 1, 2022 21:31
@MadLittleMods
Copy link
Contributor Author

Thanks for the all of the review @t3chguy! 🐝 Lots of strain to get it up to snuff but came out with a lot more polish and more confidence in this new timeline concept working!

su-ex added a commit to SchildiChat/matrix-js-sdk that referenced this pull request Jul 7, 2022
* Remove unused sessionStore ([\matrix-org#2455](matrix-org#2455)).
* Implement MSC3827: Filtering of `/publicRooms` by room type ([\matrix-org#2469](matrix-org#2469)).
* expose latestLocationEvent on beacon model ([\matrix-org#2467](matrix-org#2467)). Contributed by @kerryarchibald.
* Live location share - add start time leniency ([\matrix-org#2465](matrix-org#2465)). Contributed by @kerryarchibald.
* Log real errors and not just their messages, traces are useful ([\matrix-org#2464](matrix-org#2464)).
* Various changes to `src/crypto` files for correctness ([\matrix-org#2137](matrix-org#2137)). Contributed by @ShadowJonathan.
* Update MSC3786 implementation: Check the `state_key` ([\matrix-org#2429](matrix-org#2429)).
* Timeline needs to refresh when we see a MSC2716 marker event  ([\matrix-org#2299](matrix-org#2299)). Contributed by @MadLittleMods.
* Try to load keys from key backup when a message fails to decrypt ([\matrix-org#2373](matrix-org#2373)). Fixes element-hq/element-web#21026. Contributed by @duxovni.
* Send call version `1` as a string ([\matrix-org#2471](matrix-org#2471)). Fixes element-hq/element-web#22629.
* Fix issue with `getEventTimeline` returning undefined for thread roots in main timeline ([\matrix-org#2454](matrix-org#2454)). Fixes element-hq/element-web#22539.
* Add missing `type` property on `IAuthData` ([\matrix-org#2463](matrix-org#2463)).
* Clearly indicate that `lastReply` on a Thread can return falsy ([\matrix-org#2462](matrix-org#2462)).
* Fix issues with getEventTimeline and thread roots ([\matrix-org#2444](matrix-org#2444)). Fixes element-hq/element-web#21613.
* Live location sharing - monitor liveness of beacons yet to start ([\matrix-org#2437](matrix-org#2437)). Contributed by @kerryarchibald.
* Refactor Relations to not be per-EventTimelineSet ([\matrix-org#2412](matrix-org#2412)). Fixes matrix-org#2399 and element-hq/element-web#22298.
* Add tests for sendEvent threadId handling ([\matrix-org#2435](matrix-org#2435)). Fixes element-hq/element-web#22433.
* Make sure `encryptAndSendKeysToDevices` assumes devices are unique per-user. ([\matrix-org#2136](matrix-org#2136)). Fixes matrix-org#2135. Contributed by @ShadowJonathan.
* Don't bug the user while re-checking key backups after decryption failures ([\matrix-org#2430](matrix-org#2430)). Fixes element-hq/element-web#22416. Contributed by @duxovni.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants