Ensure deferred

[roshii]

Following #1562 (comment) this PR implements coroutines with async and await syntax.

[roshii]

It may actually fix some random CI failure of test_wallet_rpc.py which return errors when waiting for longer than 2 min.

[kristapsk]

@AdamISZ Could you take a look on this?

[AdamISZ]

tACK e550051

However, a caveat is that I can't remember if or why it's helpful to make this change?

This comment in #1562:

now that I know async syntax is not only a personal preference

I don't have a preference for async syntax, I've never used it. I could believe for sure it would be better to have the whole codebase use that; it's much more widely supported than twisted. However that is a huge job because it is not only syntax, it also affects many sub-functionalities like Tor and even the Qt GUI.

So to be clear, I don't really know the reason for this PR, but, there is no change in test functionality, and, it works (according to my tests). So if others want to merge it, I will not object (hence the ack-).

[kristapsk]

It may actually fix some random CI failure of test_wallet_rpc.py which return errors when waiting for longer than 2 min.

If this is true, that's enough reason to merge for me.

[AdamISZ]

It may actually fix some random CI failure of test_wallet_rpc.py which return errors when waiting for longer than 2 min.

If this is true, that's enough reason to merge for me.

Oh, I was going to write that myself, as I had a memory of that, but then I couldn't find the quote, so I didn't include it. Indeed, although, I'm not sure why that would be the case. If it is @roshii , can you remind us why?

[roshii]

So to be clear, I don't really know the reason for this PR, but, there is no change in test functionality, and, it works (according to my tests). So if others want to merge it, I will not object (hence the ack-).

Reason for this PR is best stated by this post, see Motivation section.

It may actually fix some random CI failure of test_wallet_rpc.py which return errors when waiting for longer than 2 min.

If this is true, that's enough reason to merge for me.

Oh, I was going to write that myself, as I had a memory of that, but then I couldn't find the quote, so I didn't include it. Indeed, although, I'm not sure why that would be the case. If it is @roshii , can you remind us why?

I did not find any explanation for this changed behavior. I did notice that some test happens to timeout after 120 s with old syntax. And timing from this Run tests step lasting longer than usual with ~16 min lead me to believe it could help solving the timeout issue.

Digging for it though, I found other case of Run tests lasting long with the old syntax... So I don´t know really if this helps. but coroutine surely do no timeout after 120s

[AdamISZ]

@roshii thanks for these thoughts, lots to chew on!

Reason for this PR is best stated by matrix-org/synapse#11777, see Motivation section.

That was an interesting read. Some of the discussion (including the follow up comments) does mention, though, that trying to mix the two is pretty problematic.

I did not find any explanation for this changed behavior. I did notice that some test happens to timeout after 120 s with old syntax.

That case is interesting. I may be reading it wrong, but i think it shows that the timeout is occurring during wallet sync (see lines 314-315); 120s is after all an absurd amount of time, especially in a test setup, for that. I was going to say "maybe merging #1594 will help", but, well, it might, but we should never need 120 seconds there, so I'm less inclined to worry about if there's a default timeout lurking somewhere because of that.

About the long timings: there are definitely some inefficiencies lurking in the test suite, but indeed, I have found some tests take an age to run, part of it, I think, is download and install is sometimes crazy slow. I see 10 minutes used up by that, in the 19 minute run you linked.

[roshii]

That was an interesting read. Some of the discussion (including the follow up comments) does mention, though, that trying to mix the two is pretty problematic.

Mixing can be problematic indeed, which is probably why it was not working when I first wrote test, mixing syntax. Therefore this PR, changing the whole unit test suite to async/await syntax, as per Twisted guidelines.
This was straight forward since the test suite remains rather small. Changing the whole API to async/await syntax will require more work and I thought it made sense to push this one independently as a small contribution towards transition.

That case is interesting. I may be reading it wrong, but i think it shows that the timeout is occurring during wallet sync (see lines 314-315); 120s is after all an absurd amount of time, especially in a test setup, for that. I was going to say "maybe merging #1594 will help", but, well, it might, but we should never need 120 seconds there, so I'm less inclined to worry about if there's a default timeout lurking somewhere because of that.

There are good arguments for default timeout, 120 s is indeed absurd. In my opinion though, it should not cause test to fail, code is working as expected. #1594 may help but if Github CI infrastructure is under pressure it may just go as slow and fail.

About the long timings: there are definitely some inefficiencies lurking in the test suite, but indeed, I have found some tests take an age to run, part of it, I think, is download and install is sometimes crazy slow. I see 10 minutes used up by that, in the 19 minute run you linked.

From what I have seen 10 min is rather standard for the setup phase. On the other hand, more than 12 min is unusual for the test phase.

Regardless, I am perfectly fine at leaving the PR as draft for now, gather more data to assess if this helps with test timeout, maybe expand its scope to a full syntax migration, or simply close it if we are just happy with Twisted Deferred. Suggestions welcome naturally.

[roshii]

rebased