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.

Sign up for GitHub

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

feat(python): makes RedisConnection's close method async, use aclose when possible #2559

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

feat(python): makes RedisConnection's close method async, use aclose when possible #2559

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
db27e00
feat(python): makes RedisConnection's close method async, use Redis a…
AIexanderDicke May 9, 2024
5d9d62c
cleanup
AIexanderDicke May 9, 2024
f52a7ff
refactor(python): await future instead of returning it
AIexanderDicke May 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions python/bullmq/flow_producer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,8 +134,8 @@ async def addBulk(self, flows: list[dict]):

return result

def close(self):
async def close(self):
"""
Close the flow instance.
"""
return self.redisConnection.close()
return await self.redisConnection.close()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to await if the result is a future if you are going to return it anyway? I wonder because in JS is not needed.

Copy link
Author

@AIexanderDicke AIexanderDicke May 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for your comment! I thought about it a little bit and I think the best solution is to get rid of the return statement completely. This way it is much clearer that you have to await the closing of the connection.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, not sure why it is clearer, for instance, is it possible that the user calls close without awaiting it?

Copy link
Author

Choose a reason for hiding this comment

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

When you see something like

async def foo():
    await bar()

it is immediately clear that you have to run await foo(). On the other hand, if you have a function

def foo():
   return bar()

it's not so clear in my opinion.

4 changes: 2 additions & 2 deletions python/bullmq/queue.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,11 +308,11 @@ def sanitizeJobTypes(self, types):
'waiting-children'
]

def close(self):
async def close(self):
"""
Close the queue instance.
"""
return self.redisConnection.close()
return await self.redisConnection.close()

def remove(self, job_id: str, opts: dict = {}):
return self.scripts.remove(job_id, opts.get("removeChildren", True))
7 changes: 5 additions & 2 deletions python/bullmq/redis_connection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,14 @@ def disconnect(self):
"""
return self.conn.disconnect()

def close(self):
async def close(self):
"""
Close the connection
"""
return self.conn.close()
try:
return await self.conn.aclose()
except AttributeError:
return self.conn.close()

async def getRedisVersion(self):
if self.version is not None:
Expand Down