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

Add EDUs to appservice transactions #1496

Closed
wants to merge 2 commits into from

Conversation

turt2live
Copy link
Member

Rendered: see 'docs' commit status
Synapse PR: coming soon


Many bridges are interested in this information, and considering they can no longer sync themselves they need a way to get EDUs. Servers should send EDUs as per the rules for "interested events" defined elsewhere.

EDUs are a new top level key for backwards compatibility and sanity. Having EDUs in the events array would likely break appservices which are not EDU-aware due to missing fields and unknown event types.

The events array is still required for backwards compatibility. The added condition of it may be empty has been added so that appservices which are not aware of EDUs don't break with a missing field. Appservices should be iterating over the list, so a zero-element list should be fine.

Fixes matrix-org/matrix-spec#270

Many bridges are interested in this information, and considering they can no longer sync themselves they need a way to get EDUs. Servers should send EDUs as per the rules for "interested events" defined elsewhere.

EDUs are a new top level key for backwards compatibility and sanity. Having EDUs in the events array would likely break appservices which are not EDU-aware due to missing fields and unknown event types. 

The events array is still required for backwards compatibility. The added condition of it may be empty has been added so that appservices which are not aware of EDUs don't break with a missing field. Appservices should be iterating over the list, so a zero-element list should be fine.
@turt2live turt2live requested a review from a team August 8, 2018 16:08
items:
type: object
title: Ephemeral Event
# TODO: Reference client schema for EDUs
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is something we haven't yet defined in the client-server spec, but would be nice to do. I'd like to fork that off to the "improve client-server spec" pile though.

"content": {
"user_ids": ["@alice:localhost"]
}
}
]
}
description: "Transaction informations"
Copy link
Member

Choose a reason for hiding this comment

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

Information*?

@turt2live
Copy link
Member Author

Just leaving a note: this is being put as r0-time-permitting and may very well end up in a post-r0 world. There's concerns about the complexity of calculations needed to handle presence for appservices, amoung other EDUs.

The complexity comes about due to presence not being bound to a room. When a server receives a presence update, it would have to cross reference rooms that the presence user is in and rooms the appservice is in, which leads to a nightmarish loop. Initial attempts to implement this calculation have shown that it can take several seconds depending on the user at hand. The fastest approach appears to be looping through the rooms the sending user is in (and known to the server), then checking the membership list of each room for an appservice - returning early if needed. Servers with multiple appservices have other optimizations they can make (iterating until all appservices are discovered in the room, or the end of the membership list is reached).

@turt2live turt2live added the blocked Something needs to be done before action can be taken on this PR/issue. label Aug 16, 2018
@turt2live
Copy link
Member Author

Closing pending MSC

@turt2live turt2live closed this Sep 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Something needs to be done before action can be taken on this PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants