Add rsloop as backend for asyncio tests.#1199
Conversation
Vizonex
left a comment
There was a problem hiding this comment.
@tapetersen I love this because it's another low level event loop that has windows support. besides just winloop and the selector system pywepoll, (both of which I wrote) I'm glad to see libraries with less neglect for windows support (excluding rloop & uvloop) this had always been a reoccurring pet-peve of mine but I'm glad to see something new that could provide a positive impact for windows users. I approve this pull request due to that fact and I hope rsloop receives positive attention.
|
@Vizonex I mostly completed what was needed to get the test-suite to pass in rsloop for curiousity. I havn't used it for something real yet and don't have any idea for how ready it is. If nothing else it would be good to see it tries in more projects if you have any. There is no need for this PR to use it just pass loop_factory=rsloop.new_event_loop to anyio.run |
@tapetersen I already added it to cyares Vizonex/cyares#91 It might be added to more places if I can think of anywhere to have it placed. |
|
Can you figure out if the timeouts happen for a reason or if the test suite needs more time to the increased number of tests? |
|
I have gotten hold of a remote windows VM and can confirm they look like real timeouts, not just slow tests. I'll see if I can figure out why. |
|
I have a real Windows machine here so I can also do this when time allows. |
|
I tried adding a |
|
Confirmed locally. |
|
Yeah, it's probably a rsloop bug of not failing when connecting to a closed socket on windows. I'll see if I can distill it and add an issue there instead. Skipping these let's the whole suite through otherwise though. Should we just mark them as xfail or wait until it's fixed upstream (which I'll try to do anyway)? |
|
Upstream issue here for the windows issue For the macos timeout I don't have a better way to test. I may report it anyway. |
I don't have a strong opinion on this, but this seems like a fairly serious bug in rsloop. I'm leaning towards waiting until the test suite passes cleanly. This is what we did with |
|
I have fixed that bug on Windows in 0.1.22 version. can we re-run CI checks? |
|
I think I managed to re-run them. There is still some error on macOS as well that I don't have a a good way to debug. It may be ok to exclude that though as long as it works on other platforms. |
|
Given that both macOS jobs failed with a |
|
I will try to figure out what's wrong with those CI runs today. |
|
@tapetersen can you re-run tests again here, please? |
|
Not sure what you expected to happen, but even more test jobs failed. |
|
@agronholm can you re-run again? in my local test - it works now with v0.1.25 |
|
Something is still off, as some tests fail due to the lack of an exception where one was expected. I confirmed that 0.1.25 is installed before the test run. |
Thank you. I will work on the remaining three failed tests. Before about five tests failed. |
|
@agronholm can you bump the version to 0.1.26, please? |
|
@egorsmkv I can so it but if you want to continue testing it may be better to just create your own PR that will be triggered when you push. I created this one to iron out the Unix bugs and see how far I came to complete. Since I'm a collaborator now I can even push to your branch if you check that flag so it may be easier to just handle that way. |
|
@tapetersen now all checks are passed so there is no need or it |
|
@tapetersen is this good for merging? I see it's still marked as a draft. |
|
@agronholm There is still the one refcycle leak on happy-eyeballs I marked as skip since it wasn't fixed and I didn't know if we eanted to take on extra test-burden when I created it but if the test-integration looks ok for you I don't mind merging it. |
| # These below may be sent with explicit default arguments and so must accept | ||
| # these literally | ||
| cwd: StrOrBytesPath | None = None, | ||
| env: Mapping[str, str] | None = None, | ||
| startupinfo: Any = None, | ||
| creationflags: int = 0, | ||
| start_new_session: bool = False, | ||
| pass_fds: Sequence[int] = (), |
There was a problem hiding this comment.
Hm, can you help me understand this change? It doesn't seem directly related to the subject of the PR.
There was a problem hiding this comment.
While trying to initially get rsloop to pass one of the failures was that it didn't accept these arguments (at least cwd and env) explicitly as None while it handled them correctly if omitted (treating them as if None was passed).
Since anyio actually passes None explicitly see https://github.com/agronholm/anyio/blob/master/src/anyio/_core/_subprocesses.py#L184 a backend is required to accept that interface (as **kwargs: Any doesn't put any requirements on them).
The bug is since fixed in rsloop so it's not required but was related. I like making these kind of things explicit in the types if possible but can also just remove it or put it in a separate MR if you prefer.
There was a problem hiding this comment.
But as long as we keep forwarding **kwargs:Any it won't really catch that much of anything with typing so I can very well see the case for removing it.
There was a problem hiding this comment.
So basically rsloop would be fine with open_process(...) and do the correct default thing for cwd and env if ommitted but not accept the explicit defaults as in open_process(..., cwd=None, env=None) as it didn't like the None type.
|
I don't think this needs a changelog entry as it's not a user-facing change. Thoughts? |
|
@agronholm I agree and remove it. How do you want to do with the extra argument annotations for They are not necessary and are a matter of taste but I tend to prefer them. Also are you ok with the skipped ref-leak test for happy eyeballs or should I unskip it and hold the PR until it's fixed in |
@egorsmkv What's your take on this? A refleak isn't that serious a problem, but if you are planning to fix that soonish, then I'd rather wait for that. |
I will try to fix that tomorrow |
This reverts commit 5494d02.
Remove the skipped test as it should be fixed upstream before merge.
|
@egorsmkv I've removed the skip in the code so it should start failing on CI again. |
|
okay, let's test with v0.1.27 |
|
@egorsmkv I've retriggered it. I think it should pick up the latest if published since we don't pin any dependencies here but of not I'll push a change with a higher bound. |
|
@tapetersen can you update to v0.1.30? i tested it in #1238 and it now passed tests |
|
@egorsmkv Done |
Changes
rsloopas an implementation of asyncio'sAbstractEveltLoophas reached full compatibility required by AnyIO's tests.This PR adds it as an alternative backend to test against along
uvloopChecklist
tests/) which would fail without your patchdocs/), in case of behavior changes or newfeatures
docs/versionhistory.rst).