Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure deferred #1565

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

roshii
Copy link
Contributor

@roshii roshii commented Oct 3, 2023

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

@roshii
Copy link
Contributor Author

roshii commented Oct 6, 2023

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

@kristapsk
Copy link
Member

@AdamISZ Could you take a look on this?

@AdamISZ
Copy link
Member

AdamISZ commented Nov 22, 2023

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
Copy link
Member

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
Copy link
Member

AdamISZ commented Nov 22, 2023

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
Copy link
Contributor Author

roshii commented Nov 28, 2023

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
Copy link
Member

AdamISZ commented Dec 1, 2023

@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
Copy link
Contributor Author

roshii commented Dec 5, 2023

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
Copy link
Contributor Author

roshii commented Feb 23, 2024

rebased

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants