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

Better fallback for the event localTimestamp calculation. #3900

Merged
merged 9 commits into from
Nov 20, 2023

Conversation

toger5
Copy link
Contributor

@toger5 toger5 commented Nov 17, 2023

Signed-off-by: Timo K [email protected]
State events from a couple of home servers dont return an age proptery in their unsigned field. (including element.io).
For an actual exampe, this room: https://matrix.to/#/#thisweekinmatrix:matrix.org can be checked (lots of member events from other homeservers are not featuring a unsigned.age field):
Screenshot 2023-11-17 at 19 36 16

Call state events are relying on this property to determin if they are expired or not.

Before a non existent age property in the unsigned field resulted in falling back to 0. The localTimestamp was then Date.now() at the time of loading this event. Which made it always really recent -> never expired.

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

Here's what your changelog entry will look like:

✨ Features

  • Better fallback for the event localTimestamp calculation. (#3900). Contributed by @toger5.

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

  • Please could you update the subject of the PR to better explain what is going on. Better than what? What even is a localTimestamp? Basically, I shouldn't have to read the diff to get a rough idea of what a PR does.

  • You say this "fixes issues with call expiration". Are these issues tracked anywhere? If so, please link them. If not, please describe them. Basically, I should know what it is you're trying to fix.

  • I continue to be very surprised that you are actually seeing call events that lack an age. Please could you share a /sync result or somesuch that demonstrates this?

src/models/event.ts Outdated Show resolved Hide resolved
src/matrixrtc/CallMembership.ts Show resolved Hide resolved
spec/unit/matrixrtc/mocks.ts Outdated Show resolved Hide resolved
@toger5
Copy link
Contributor Author

toger5 commented Nov 17, 2023

  • Please could you update the subject of the PR to better explain what is going on. Better than what? What even is a localTimestamp? Basically, I shouldn't have to read the diff to get a rough idea of what a PR does.

I searched and found a specific example.

* You say this "fixes issues with call expiration". Are these issues tracked anywhere? If so, please link them. If not, please describe them. Basically, I should know what it is you're trying to fix.

Not officially tracked on GH but there are reports in various matrix rooms.

* I continue to be very surprised that you are actually seeing call events that lack an `age`. Please could you share a `/sync` result or somesuch that demonstrates this?

This seemed to be the case for all state events whcih are received from another homeserver. (All is hard to prove but most of the federated ones I have checked dont have a unsigned.age field)

Edit:
I did some more trying and sometimes federated state events also have an age field.

@toger5 toger5 requested a review from richvdh November 17, 2023 18:40
@toger5 toger5 changed the title Better localTimestamp fallback Better fallback for the event localTimestamp calculation. Nov 17, 2023
@richvdh
Copy link
Member

richvdh commented Nov 19, 2023

* I continue to be very surprised that you are actually seeing call events that lack an `age`. Please could you share a `/sync` result or somesuch that demonstrates this?

This seemed to be the case for all state events whcih are received from another homeserver. (All is hard to prove but most of the federated ones I have checked dont have a unsigned.age field)

Edit: I did some more trying and sometimes federated state events also have an age field.

Ah right, it affects federated events. In that case it sounds like a likely effect of matrix-org/synapse#8429. Why don't we just fix that?

@fkwp
Copy link

fkwp commented Nov 20, 2023

* I continue to be very surprised that you are actually seeing call events that lack an `age`. Please could you share a `/sync` result or somesuch that demonstrates this?

This seemed to be the case for all state events whcih are received from another homeserver. (All is hard to prove but most of the federated ones I have checked dont have a unsigned.age field)
Edit: I did some more trying and sometimes federated state events also have an age field.

Ah right, it affects federated events. In that case it sounds like a likely effect of matrix-org/synapse#8429. Why don't we just fix that?

Maybe a robust design is favorable in case you are using a homeserver with that specific "bug".

@toger5
Copy link
Contributor Author

toger5 commented Nov 20, 2023

Thanks for the reply. Fixing it in synapse definitly would be the more desirable solution. I still advocat having a frontend fallback solution. Otherwise we would rely on all homeserver maintainers to update to the patched version and until then calling in EW looks broken. (It would always show ongoing calls for federated event if ppl dont properly close their call.)

Since we cannot guarantee a date until which all homeserver maintainers have patched their synapse this fallback might be helpful for a long enough time to make it worthit.

If someone can make time to fix the synapse issue as well that would be very appreciated. (I dont know if this fits as a task for me for our current goals.)

@richvdh
Copy link
Member

richvdh commented Nov 20, 2023

Ah right, it affects federated events. In that case it sounds like a likely effect of matrix-org/synapse#8429. Why don't we just fix that?

Maybe a robust design is favorable in case you are using a homeserver with that specific "bug".

Yeah I don't object to fixing both sides of it (indeed one will probably have to since it will take a while for a Synapse fix to roll out). But just fixing the client side does feel a lot like curing the symptoms rather than the disease. Having an entire stack which is full of workarounds for bugs in other parts of the stack does not lead to a stable, maintainable system.

src/models/event.ts Outdated Show resolved Hide resolved
src/models/event.ts Outdated Show resolved Hide resolved
Co-authored-by: Richard van der Hoff <[email protected]>
spec/unit/matrixrtc/mocks.ts Outdated Show resolved Hide resolved
spec/unit/matrixrtc/mocks.ts Outdated Show resolved Hide resolved
spec/unit/matrixrtc/mocks.ts Outdated Show resolved Hide resolved
spec/unit/matrixrtc/MatrixRTCSession.spec.ts Show resolved Hide resolved
spec/unit/matrixrtc/mocks.ts Outdated Show resolved Hide resolved
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

just a few nits, generally looks good!

Co-authored-by: Richard van der Hoff <[email protected]>
So that the localTimestamp and localAge behavior is easier to understand.

Signed-off-by: Timo K <[email protected]>
with binding the whole roomState

Signed-off-by: Timo K <[email protected]>
@toger5 toger5 requested a review from richvdh November 20, 2023 15:35
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm!

@toger5 toger5 enabled auto-merge November 20, 2023 16:54
@toger5 toger5 added this pull request to the merge queue Nov 20, 2023
@robintown robintown removed the request for review from florianduros November 20, 2023 17:20
Merged via the queue into develop with commit e98ce78 Nov 20, 2023
23 checks passed
@toger5 toger5 deleted the toger5/better-localTimestamp-fallback branch November 20, 2023 18:04
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.

4 participants