matrix-org / synapse

Synapse: Matrix homeserver written in Python/Twisted.
https://matrix-org.github.io/synapse
Apache License 2.0
11.83k stars 2.13k forks source link

Fix tests on Twisted trunk. #16528

Closed clokep closed 1 year ago

clokep commented 1 year ago

Part of #16289, before fixing them we need to refactor some code first.

Twisted trunk makes a change to the TLSMemoryBIOFactory where the underlying protocol is changed from TLSMemoryBIOProtocol to BufferingTLSTransport to improve performance of TLS code (see https://github.com/twisted/twisted/issues/11989). In order to properly hook up this code we need to modify some of our test code to pass the test reactor's clock into TLSMemoryBIOFactory such that the global (trial) reactor isn't used by default.

This is all rather annoying, especially when attempting to maintain compatible with older Twisted versions.

See https://github.com/twisted/twisted/pull/11996/files#diff-9d27cdf01b21540c6ef350193a9d44b88a3d2b322f3fbc08b95cdaac8fd50ed6R871-R874 for where Twisted does this in its own tests.

DMRobertson commented 1 year ago

Looks sane, but I don't understand what exactly was failing and why on Twisted trunk. Could you spell it out for me?

Oh, I think I see: https://github.com/twisted/twisted/pull/11996 made speedups in twisted. But as a consequence of how they did so, their tests now need to pass in a Clock explicitly. We need to do the same.

clokep commented 1 year ago

Looks sane, but I don't understand what exactly was failing and why on Twisted trunk. Could you spell it out for me?

I attempted to add more info to the description, I'm not sure it is enough though.

DMRobertson commented 1 year ago

Thanks! I think I was confused because the version check we added made me think there was a behaviour change in 23.8.0, rather than in twisted trunk.

clokep commented 1 year ago

Thanks! I think I was confused because the version check we added made me think there was a behaviour change in 23.8.0, rather than in twisted trunk.

There's no released version so I had to do checks against the currently released version (and hope there's no 23.8.x release.... but I find that unlikely).

clokep commented 1 year ago

Looks like the patching is causing issues... maybe a memory leak?

clokep commented 1 year ago

I kicked off a run without the patching: https://github.com/matrix-org/synapse/actions/runs/6626203043

clokep commented 1 year ago

I kicked off a run without the patching: matrix-org/synapse/actions/runs/6626203043

Looks like the tests do fail without this change, I wonder if they pass on the commit before?

clokep commented 1 year ago

https://github.com/matrix-org/synapse/actions/runs/6626962438

clokep commented 1 year ago

matrix-org/synapse/actions/runs/6626962438

This passed, but fails on older Twisteds, I think we should just not patch older ones then? 🤷

clokep commented 1 year ago

Tests passed here, now let's see if they pass on Twisted Trunk: https://github.com/matrix-org/synapse/actions/runs/6628632702

clokep commented 1 year ago

Let's try again: https://github.com/matrix-org/synapse/actions/runs/6630078646

clokep commented 1 year ago

@DMRobertson Can you take another quick look?