Update implementation of MSC4306: Thread Subscriptions to include automatic subscription conflict prevention as introduced in later drafts. #18756
Conversation
| # Find the maximum stream ordering and topological ordering of the room, | ||
| # which we then store against this unsubscription so we can skip future | ||
| # automatic subscriptions that are caused by an event logically earlier | ||
| # than this unsubscription. | ||
| txn.execute( | ||
| """ | ||
| SELECT MAX(stream_ordering) AS mso, MAX(topological_ordering) AS mto FROM events | ||
| WHERE room_id = ? | ||
| """, | ||
| (room_id,), | ||
| ) |
There was a problem hiding this comment.
Looking up the max stream_ordering and topological_ordering on the server at the time of unsubscription has the flaw that if the client has an outdated view of the world and sends an unsubscribe, we will be unsubscribed even though other clients may see new mentions in the thread that they want to subscribe to.
There was a problem hiding this comment.
I get this, but also entirely on the fence about it, because I feel like both answers can be right or wrong.
One thought:
When a user clicks unsubscribe, they mean unsubscribe. They might feel it unexpected to click unsubscribe at 15:09 and be still subscribed because their client didn't see a message at 15:08.
The other thought:
A user might expect what you describe and not have their input be counted against things they haven't seen yet.
Overall I feel like:
- this kind of race condition is not super common
- working offline (i.e. client being fully offline and displaying threads with hours/days of time lag and being able to queue up unsubscriptions whilst in this state) is the most realistic case that would cause this, but today's clients are not really operating well in this area and the client could implement smarter logic when it does come online to mitigate this problem.
- most clients will only be implementing unsubscriptions in real time (with network access), so most likely you will have tens of seconds of lag at maximum
- I think users could be forgiving of either case.
Maybe this is something that would be better as an MSC comment, but at the end of the day implementations have to choose one side of the fence to fall, nothing seems perfect.
There was a problem hiding this comment.
Commented on the MSC: matrix-org/matrix-spec-proposals#4306 (comment)
In terms of pushing this forward and merging this PR, since this is an experimental MSC, it can be as flawed as we desire and I can push back on whatever flaws on the MSC itself. And I think you're explanation of both sides is good enough to justify trying out this way for now.
| COMMENT ON COLUMN thread_subscriptions.unsubscribed_at_stream_ordering IS | ||
| $$The maximum stream_ordering in the room when the unsubscription was made.$$; | ||
|
|
||
| COMMENT ON COLUMN thread_subscriptions.unsubscribed_at_topological_ordering IS | ||
| $$The maximum topological_ordering in the room when the unsubscription was made.$$; |
Co-authored-by: Eric Eastwood <erice@element.io>
Co-authored-by: Eric Eastwood <erice@element.io>
reivilibre
force-pushed
the
rei/t2_msc4306_conflict
branch
from
bf930f3 to
956532a
Compare
reivilibre
force-pushed
the
rei/t2_msc4306_conflict
branch
from
8b1072c to
e90af84
Compare
reivilibre
force-pushed
the
rei/t2_msc4306_conflict
branch
from
97120bc to
eb0f204
Compare
reivilibre
force-pushed
the
rei/t2_msc4306_conflict
branch
from
aed6d75 to
29c1ae4
Compare
Follows: #18674
Implements new drafts of MSC4306
Add a
AnyEventIdpydantic-validated typeAdd a test helper for sending a sequence of events
Add conflict prevention to automatic thread subscriptions