Skip to content

PYTHON-5089 Convert test.test_mongos_load_balancing to async #2107

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

Merged
merged 11 commits into from
Feb 6, 2025
25 changes: 12 additions & 13 deletions test/asynchronous/test_mongos_load_balancing.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,17 +34,11 @@
_IS_SYNC = False


@async_client_context.require_connection
@async_client_context.require_no_load_balancer
def asyncSetUpModule():
pass


if not _IS_SYNC:
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's standardize these checks to if _IS_SYNC for clarity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay yes, that's what I wanted (and did initially) but for some reason if its if _IS_SYNC first, type-checker assumes both definitions of SimpleOp inherit from threading.Thread and then insist that both implementations adhere to the thread api (in this case join must return a value)
is that preferred? it felt weird to return a value for the sake of it in the async version of SimpleOp simply because its not used at all

Copy link
Contributor

Choose a reason for hiding this comment

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

Our typing checks do, or the IDE's own highlighting does? Either way, we should have all checks be for _IS_SYNC whenever possible for clarity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

both, our typing checks is what caught my attention first actually


class SimpleOp:
def __init__(self, client):
self.task: asyncio.Task
self.task = asyncio.create_task(self.run())
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer the earlier version, sorry for the churn.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope all good, i offered it as an alternative :)

self.client = client
self.passed = False

Expand All @@ -53,7 +47,7 @@ async def run(self):
self.passed = True # No exception raised.

def start(self):
self.task = asyncio.create_task(self.run())
pass

async def join(self):
await self.task
Expand All @@ -70,15 +64,15 @@ def run(self):
self.passed = True # No exception raised.


async def do_simple_op(client, nthreads):
threads = [SimpleOp(client) for _ in range(nthreads)]
for t in threads:
async def do_simple_op(client, ntasks):
tasks = [SimpleOp(client) for _ in range(ntasks)]
for t in tasks:
t.start()

for t in threads:
for t in tasks:
await t.join()

for t in threads:
for t in tasks:
assert t.passed


Expand All @@ -90,6 +84,11 @@ async def writable_addresses(topology):


class TestMongosLoadBalancing(AsyncMockClientTest):
@async_client_context.require_connection
@async_client_context.require_no_load_balancer
async def asyncSetUp(self):
await super().asyncSetUp()

def mock_client(self, **kwargs):
mock_client = AsyncMockClient(
standalones=[],
Expand Down
25 changes: 12 additions & 13 deletions test/test_mongos_load_balancing.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,17 +34,11 @@
_IS_SYNC = True


@client_context.require_connection
@client_context.require_no_load_balancer
def setUpModule():
Copy link
Contributor

Choose a reason for hiding this comment

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

We can keep these to improve performance when skipping this test suite.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved it to the class's setUp because on the async side it'd require setUpModule to be awaited and i couldn't find an easy way to achieve that. I figured it was the only class in this module so it didn't make a difference to just move it into the class. But if you know how i could await this in async, I have no hesitations to bring it back!

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh whoops my bad, forgot how the wrappers interact with async. You're right!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All good, wasn't sure if I was missing / forgetting what the trick was!

pass


if not _IS_SYNC:

class SimpleOp:
def __init__(self, client):
self.task: asyncio.Task
self.task = asyncio.create_task(self.run())
self.client = client
self.passed = False

Expand All @@ -53,7 +47,7 @@ def run(self):
self.passed = True # No exception raised.

def start(self):
self.task = asyncio.create_task(self.run())
pass

def join(self):
self.task
Expand All @@ -70,15 +64,15 @@ def run(self):
self.passed = True # No exception raised.
Copy link
Member

Choose a reason for hiding this comment

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

Can we settle on a common pattern for the threading.Thread -> Task conversions? I made some suggestions in #2103.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thoughts on the approach in #2094?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I believe if I implement noah's suggestion above we'd be using the same pattern :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(i think my tab wasn't fully refreshed earlier so i'm just seeing Noah's comment right now) but I actually really like that approach!



def do_simple_op(client, nthreads):
threads = [SimpleOp(client) for _ in range(nthreads)]
for t in threads:
def do_simple_op(client, ntasks):
tasks = [SimpleOp(client) for _ in range(ntasks)]
for t in tasks:
t.start()

for t in threads:
for t in tasks:
t.join()

for t in threads:
for t in tasks:
assert t.passed


Expand All @@ -90,6 +84,11 @@ def writable_addresses(topology):


class TestMongosLoadBalancing(MockClientTest):
@client_context.require_connection
@client_context.require_no_load_balancer
def setUp(self):
super().setUp()

def mock_client(self, **kwargs):
mock_client = MockClient(
standalones=[],
Expand Down
Loading