Refactor MatrixFederationAgent to retry SRV.

[erikjohnston]

This refactors MatrixFederationAgent to move the SRV lookup into the
endpoint code, this has two benefits:

1. Its easier to retry different host/ports in the same way as HostnameEndpoint.
2. We avoid SRV lookups if we have a free connection in the pool

[erikjohnston]

So if I use make_deferred_yieldable here I get lots of failures due to log contexts having been changed, even when removing all log context logic from the custom endpoint and the SRV resolver

[richvdh]

ok, so this comes down to a mixup between the two types of function: (A) "follows synapse logcontext rules" vs (B) "ignores logcontext". Currently MatrixHostnameEndpoint.connect is a type-A function, whereas the thing that calls it (the depths of Twisted) is a type-B function. The TL;DR is that this doesn't work (in more detail: MatrixHostnameEndpoint.connect() returns a Deferred which reinstates the logcontext when it completes, but the Twisted code which calls it isn't expecting that, so the logcontext ends up ending up in places it shouldn't, and everything falls apart).

So one solution is to make MatrixHostnameEndpoint.connect a type-B function instead, which would work at runtime, but you would get errors from the patched inlineCallbacks which is installed by the unit tests to check that all the synapse functions are law-abiding type-A functions.

So a solution to this would be to make connect a thin (type-B) shim which calls a type-A _do_connect (or whatever) method which uses inlineCallbacks. Thus:

class MatrixHostnameEndpoint(object):

    def connect(self, protocol_factory):
        return run_in_background(self._do_connect, protocol_factory)

    @defer.inlineCallbacks
    def _do_connect(self, protocol_factory):
        # ... etc

You should then be able to use make_deferred_yieldable here instead of dropping the logcontext. The nice thing about that is that the logcontext will be then in place for the whole of _do_connect, so everything that endpoint.connect and the srv resolver log will end up with the right request id.

It'll probably still fail, but try it and let's have a look.

[richvdh]

some initial thoughts

[richvdh]

there's an off-by-one error here which favours the first entry in the list. Given two servers with weights (10, 10), we should choose the first server for randoms in the range [1,10] and the second for randoms in the range [11, 20]. Currently we additionally choose the first server for random 0.

Suggested change

	target_weight = random.randint(0, total_weight)
	target_weight = random.randint(1, total_weight)

(or switch to a 0-based calculation with randrange and change line 106)

[erikjohnston]

Turns out this algorithm is the same as the one in the RFC2782, so I've documented it as such.

[richvdh]

I think the off-by-one error is still present?

[erikjohnston]

Unfortunately its complicated to change the algorithm to work with zero weights, as eventually you end up trying to do randint(1, 0) and everything blows up. I don't know whether we want to use a different algorithm which is correctly weighted, or just accept the off by one.

[richvdh]

wait, the RFC2782 algorithm has the off-by-one error? that's... bonkers.

To be clear: if you have two servers, each with a weight of 1, we'll currently use one twice as often as the other.

can we just special-case total_weight == 0? (always return the first server, or something)

[erikjohnston]

Quite. So the RFC says that its valid to have multiple servers that have a weight of zero, which means:

In the presence of records containing weights greater than 0, records with weight 0 should have a very small chance of being selected.

We could just always order weights of zero last, and shuffle them randomly if there are multiple of them? That I think should work, though ignores that section of RFC a bit

[richvdh]

We could just always order weights of zero last, and shuffle them randomly if there are multiple of them? That I think should work, though ignores that section of RFC a bit

it doesn't matter if we put them at the start or the end: a server with a weight of 0 has a 0% chance of being used in a sane algorithm. Perhaps we should do randint(0, total_weight) if there are any servers with a zero weight, and randint(1, total_weight) otherwise?

I guess it doesn't really matter, since I doubt anyone is really using SRV records to load-balance servers...

[erikjohnston]

The idea is that synapse will sequentially try each server on the list until it finds one it can connect to, so putting the zero weights at the end of the returned list means that they will get tried last (modulo priority).

[richvdh]

ohhh, right. Fair enough. sgtm then.

[richvdh]

ok, so this comes down to a mixup between the two types of function: (A) "follows synapse logcontext rules" vs (B) "ignores logcontext". Currently MatrixHostnameEndpoint.connect is a type-A function, whereas the thing that calls it (the depths of Twisted) is a type-B function. The TL;DR is that this doesn't work (in more detail: MatrixHostnameEndpoint.connect() returns a Deferred which reinstates the logcontext when it completes, but the Twisted code which calls it isn't expecting that, so the logcontext ends up ending up in places it shouldn't, and everything falls apart).

So one solution is to make MatrixHostnameEndpoint.connect a type-B function instead, which would work at runtime, but you would get errors from the patched inlineCallbacks which is installed by the unit tests to check that all the synapse functions are law-abiding type-A functions.

So a solution to this would be to make connect a thin (type-B) shim which calls a type-A _do_connect (or whatever) method which uses inlineCallbacks. Thus:

class MatrixHostnameEndpoint(object):

    def connect(self, protocol_factory):
        return run_in_background(self._do_connect, protocol_factory)

    @defer.inlineCallbacks
    def _do_connect(self, protocol_factory):
        # ... etc

You should then be able to use make_deferred_yieldable here instead of dropping the logcontext. The nice thing about that is that the logcontext will be then in place for the whole of _do_connect, so everything that endpoint.connect and the srv resolver log will end up with the right request id.

It'll probably still fail, but try it and let's have a look.

[richvdh]

I think the off-by-one error is still present?

[richvdh]

lgtm! 🎉