Skip to content

Conversation

versusvoid
Copy link
Contributor

No description provided.

Copy link
Member

@fantix fantix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the unstable test issue, I'm okay with this PR. Thanks!

Comment on lines 165 to 166
# Let all transports shutdown.
await asyncio.sleep(0.1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using sleep is generally not recommended for new tests in uvloop - this test is actually unstable, I could get a false OK if the srv.close() got called before the connection is fully established.

A proper fix is to use a Future, set in factory() and awaited in test() - we could even pass over the assertion error in this manner. With this fix, the ResourceWarning is also gone.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, fixed

Copy link
Member

@fantix fantix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see suggested changes below - could you please also rebase your commits to latest master so that the test suite may pass? Thanks!

@1st1
Copy link
Member

1st1 commented Mar 31, 2020

This particular fix is OK, but we need to fix this for all other "native" callbacks in uvloop in a more systematic way.

fantix pushed a commit to fantix/uvloop that referenced this pull request May 25, 2020
fantix pushed a commit to fantix/uvloop that referenced this pull request Jan 21, 2021
fantix pushed a commit to fantix/uvloop that referenced this pull request Jan 22, 2021
@fantix
Copy link
Member

fantix commented Feb 5, 2021

Merged in #348. Thanks for the PR!

@fantix fantix closed this Feb 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants