This repository was archived by the owner on Apr 26, 2024. It is now read-only.
Fix providing a RoomStreamToken instance to _notify_app_services_ephemeral
#11137
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…meral Previously, if a RoomStreamToken object were provided, new_token would be set to `None`. In the same changeset, which was intended to prevent mypy from failing, `_notify_app_services_ephemeral` was configured to allow `new_token` to be an Optional[int] (559974f). However, if `None` is provided, `_notify_app_services_ephemeral` will end up passing `None` to `set_type_stream_id_for_appservice`, which will effectively clear each appservice's stream token for the given `stream_key`. This seems like a bug to do every time we have a RoomStreamToken rather than an int. Instead, I'm converting the RoomStreamToken to an int using RoomStreamToken.stream. I think this is the right way to convert to an int. If RoomStreamToken this ends up being a vector clock, `stream` will be the minimum stream token amongst all workers... which I think is fine?
anoadragon453
changed the title
RoomStreamToken instance to _notify_app_services_ephemeral
anoadragon453
commented
Oct 20, 2021
DMRobertson
reviewed
Oct 21, 2021
There was a problem hiding this comment.
Code changes look sensible. CI is happy.
Instead, I propose converting the RoomStreamToken to an int using RoomStreamToken.stream (I think this is the right way to convert it. RoomStreamToken can end up being a vector clock when you have multiple event persister workers; in this case stream will be the minimum stream token amongst all workers... which I think is fine... maybe?)
Not sure on this. One for @erikjohnston or @richvdh ?
…_as_room_stream_token
richvdh
suggested changes
Oct 21, 2021
While searching around for all types that were eventually passed into on_new_event, I found a couple methods which didn't specify the type they returned. So this commit adds return type hints for those methods. I didn't do any other arguments as I plan to type-hint those files later anyways.
…instead This is a small effort to reduce the layers of methods we have here. Also, I've opted to continue converting the RoomStreamToken to an int, but only for when tracking stream tokens of EDUs. As stated in the comment, this shouldn't have any gaps as long as we *always* convert to minimum value.
…_as_room_stream_token
richvdh
suggested changes
Oct 29, 2021
erikjohnston
added
the
X-Awaiting-Changes
anoadragon453
force-pushed
the
anoa/e2e_as_room_stream_token
branch
from
b70ceca to
4414e22
Compare
anoadragon453
removed
the
X-Awaiting-Changes
Co-authored-by: Richard van der Hoff <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We typically figure out which EDUs to send to application services based the last stream token that appservice successfully received. This is true for read receipts and user presence, however for typing we always just send the last typing event. This process is started whenever a new read receipt, presence update or typing event is processed by the server. In that case,
notify_interested_services_ephemeralwill be called with the new event and its stream token (so we can check whether we need to send any other events to the appservice may have missed).With all that in mind, I believe I've found a bug in the implementation. If a
RoomStreamTokenobject was provided as thenew_tokenargument to_notify_app_services_ephemeral,new_tokenwould be set toNone, before then being passed tonotify_interested_services_ephemeraland then_notify_interested_services_ephemeral. The commit that introduced this change (559974f) was intended to prevent mypy from failing.(_)notify_app_services_ephemeralwas also configured to allownew_tokento be anOptional[int]as a result.However, if
Noneis provided,_notify_app_services_ephemeralwill end up passingNonetoset_type_stream_id_for_appservice, which will clear each appservice's stream token for the givenstream_key.Doing so means that we send all presence or read receipt data from a None stream token up until the latest token(!). This seems like a bug to do every time we have aActually, all stream keys that are passed toRoomStreamTokenrather than anint._notify_interested_services_ephemeral(typing_key,receipt_key,presence_key) are passingintvalues fornew_token, so the buggy code would have never fired anyways.Regardless, I propose converting the
RoomStreamTokento anintusingRoomStreamToken.stream(I think this is the right way to convert it.This appears to be correct, we do it elsewhere as well.)RoomStreamTokencan end up being a vector clock when you have multiple event persister workers; in this casestreamwill be the minimum stream token amongst all workers... which I think is fine... maybe?This may relate to the large bursts of EDU traffic Synapse is sending to AS's ala #10836.See above. The code should not have actually resulted in broken behaviour.