Add a get_next_txn method to StreamIdGenerator to match MultiWriterIdGenerator (#15191

[anoadragon453]

This allows for fetching the next stream ID while within a DB transaction, where one is not able to call get_next due to not being able to call async methods from within a database txn.

Additional context in commit messages.

Split out from #14869. Includes a couple docstring fixes I noticed in the area.

[squahtx]

Looks good!

[clokep]

I guess this is no longer true and we're always a writer?

[anoadragon453]

Mm, no, not all workers are account_data stream writers. But self._account_data_id_gen is always a AbstractStreamIdGenerator, even on non-writers.

The methods that rely on self._account_data_id_gen being a AbstractStreamIdGenerator should not be called unless we are a writer though. Hence the asserts in those methods, such as:

synapse/synapse/storage/databases/main/account_data.py

Line 772 in c8665dd

assert self._can_write_to_account_data

Defining self._account_data_id_gen as a AbstractStreamIdTracker confused mypy though, as AbstractStreamIdTracker doesn't have the methods that a writer need (get_next, get_next_txn, etc.).

mypy didn't yell about get_next not existing on AbstractStreamIdTracker though, possibly because we used it in async with ... blocks instead of calling it directly? Does mypy check those properly?

[clokep]

It looks like #14468 essentially removed AbstractStreamIdTracker's concrete classes without removing the abstract class, which seems unfortunate.

[anoadragon453]

It seems there's some confusion about making sequence generators AbstractStreamIdTracker in *WorkerStore classes, where it's assumed that the same sequence generator would be an AbstractStreamIdGenerator in the non-worker variant storage class. See ReceiptsWorkerStore for instance:

synapse/synapse/storage/databases/main/receipts.py

Lines 66 to 68 in 9bb2eac

	# In the worker store this is an ID tracker which we overwrite in the non-worker
	# class below that is used on the main process.
	self._receipts_id_gen: AbstractStreamIdTracker

But *WorkerStore doesn't mean that that storage class can't write to the database. It just means that workers should use that class vs. the main process... right?

[anoadragon453]

Also that comment is wrong, ReceiptsStore is just an empty class 🤦

synapse/synapse/storage/databases/main/receipts.py

Lines 1066 to 1067 in 9bb2eac

	class ReceiptsStore(ReceiptsWorkerStore, ReceiptsBackgroundUpdateStore):
	pass

Looks like there may be a few cases where AbstractStreamIdTrackers should actually be typed asAbstractStreamIdGenerators.

[clokep]

It seems there's some confusion about making sequence generators AbstractStreamIdTracker in *WorkerStore classes, where it's assumed that the same sequence generator would be an AbstractStreamIdGenerator in the non-worker variant storage class. See ReceiptsWorkerStore for instance:

#14468 made this obsolete as far as I can tell. So they just never got updated. 🤷

[clokep]

See #15192.

[anoadragon453]

CI failure appears to be a flaky test. Merging manually.