Remove concept of a non-limited stream.

[erikjohnston]

This makes the replication streams implementation simpler.

Based on #7010.

[richvdh]

I'm assuming we didn't do this for the good of our health, but rather to combat race conditions between the streams. What has changed so that it is safe to get rid of it now?

[erikjohnston]

So, I've been trying to get my head around what this is protecting and I think I've understood a little bit here. Couple of potential reasons for having this:

1. Helps preserve a bit of ordering across different streams, e.g. if there's a typing notification and then a message the synchrotrons are likely to get told about the typing notification and then the message.
2. Some streams share the same ID generator/token (e.g. the two tag streams). We want to make sure that we send down the updates for both streams up to the same token so that any stream change caches get correctly updated before anything queries for changes up to the new current token.

Now the slight problem here is that while we do ensure we update the tokens of all streams in lockstep, we then send down updates batched by stream (rather than trying to interleave the streams so that earlier updates go first). This means that the race conditions above can still happen anyway, AFAICT.

The fact that item two above is still racy sounds like a bug that needs to be fixed either way, probably easiest by merging the relevant streams so that there is a one to one mapping between streams and ID generators (I think its only global vs room account data streams and devices and user signature streams).

For item one, I'm not convinced that updating the current token in lock step actually helps that much. Ideally in each loop there will only be one or two updates to send and so the lock step does nothing, while conversely if there are a lot of updates to send down the loop workers will see updates out of order between streams anyway as we send down updates by stream.

I'm therefore minded to keep the removal of this code simply because I think it makes things a bit clearer and easier to reason about.

Thoughts welcome though, as I feel like I might still be missing something,

[richvdh]

well, it sounds pretty plausible...

[richvdh]

eyebrow raised: we're just dropping updates beyond the limit?

[erikjohnston]

The current situation is that the replication sees that it is limited and panics. The next episode PR in the series changes it so that if the stream is limited then it gets the token from the last item in the returned list

[richvdh]

(ftr: the relevant PR is #7024)

[richvdh]

conclusions from discussion elsewhere:

• the fact that we ignore updates between limit and current_id is not unique to this function: all of the update_functions do the same thing (it just looks a bit different here because the typing stream is in-memory)
• if we hit the limit, then we will throw an exception and disconnect the stream:

  synapse/synapse/replication/tcp/streams/_base.py

  Line 200 in 1410dab

  if len(updates) >= MAX_EVENTS_BEHIND:

• slicing the result here is mostly done for consistency with the other update_functions

[richvdh]

further discussion:

• slicing the result here is mostly done for consistency with the other update_functions

... but also because we're going to rely on the update_functions honouring their limit in a future PR, so we should bring this one in line now even though it's not technically necessary yet.

[richvdh]

lgtm, modulo the CI fail