Remove asgiref dependency from non Django code #1226
Conversation
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
procrastinate/utils.py
Outdated
| @@ -96,15 +95,19 @@ def async_to_sync(awaitable: Callable[..., Awaitable[T]], *args, **kwargs) -> T: | |||
| Given a callable returning an awaitable, call the callable, await it | |||
| synchronously. Returns the result after it's done. | |||
| """ | |||
| return sync.async_to_sync(awaitable)(*args, **kwargs) | |||
|
|
|||
| async def wrapper() -> T: | |||
There was a problem hiding this comment.
Not sure if there is a nicer way ... just calming down the pyright gods.
There was a problem hiding this comment.
Might work if this util (as well as the sync_to_async) is removed. By using asyncio.run directly, that should infer types correctly.
There was a problem hiding this comment.
From the asyncio docs:
This function should be used as a main entry point for asyncio programs, and should ideally only be called once. It is recommended to use loop_factory to configure the event loop instead of policies
I don't think it is a good idea to hide asyncio.run under this util for the reason above.
Looking at where it gets used:
procrastinate/procrastinate/shell.py
Line 70 in df98e61
The Shell code might need to be refactored a bit to use asyncio.run at the top level and not within loop. I believe this is what @ewjoachim mentions in another comment.
procrastinate/procrastinate/connector.py
Line 93 in 98fd11e
If I have not missed something, there is no class inheriting the base async connector that doesn't redefine the sync operations.
In that case, it might be those function implementations could be removed entirely.
In the event user code was to inherit from that class for their own custom connector, it is worth adding a note in the breaking changes section for v3.
There was a problem hiding this comment.
Might work if this util (as well as the sync_to_async) is removed. By using asyncio.run directly, that should infer types correctly.
Yes, I think so too. Those utility functions wrap only one code line each.
The Shell code might need to be refactored a bit to use
asyncio.runat the top level and not within loop. I believe this is what @ewjoachim mentions in another comment.
Yes, makes sense. I will refactor it. Another objection of @ewjoachim is (if I understood it correctly) that when you call asyncio.run inside a sync task, it will create a new event loop that is different from the main event loop (that of the worker). This isn't the case with asgiref, which ensures the same event loop is reused. But I think we should just add a warning to the documentation. It is how asyncio works.
procrastinate/procrastinate/connector.py
Line 93 in 98fd11e
If I have not missed something, there is no class inheriting the base async connector that doesn't redefine the sync operations. In that case, it might be those function implementations could be removed entirely. In the event user code was to inherit from that class for their own custom connector, it is worth adding a note in the breaking changes section for v3.
Ok, but let's tackle this in another PR.
| you will likely get a `RuntimeError`. | ||
| operations. This will only work if you call it inside a function that runs | ||
| in its own thread (such as inside a sync job). Otherwise, you will likely | ||
| get a `RuntimeError`. |
There was a problem hiding this comment.
I am not sure if this statement is still valid.
| @@ -233,7 +233,7 @@ async def ensure_async() -> Callable[..., Awaitable]: | |||
| if inspect.iscoroutinefunction(task.func): | |||
| await_func = task | |||
| else: | |||
| await_func = functools.partial(utils.sync_to_async, task) | |||
| await_func = functools.partial(asyncio.to_thread, task) | |||
There was a problem hiding this comment.
It will be interesting to discover if doing that causes issues with Django applications (https://docs.djangoproject.com/en/5.1/topics/async/#async-safety).
Also, according to https://docs.djangoproject.com/en/5.1/topics/async/#sync-to-async , the use of asgiref.sync_to_async with thread_sensitive=True (the default) needs to be done in a async_to_sync wrapper to work correctly.
There was a problem hiding this comment.
It will be interesting to discover if doing that causes issues with Django applications (https://docs.djangoproject.com/en/5.1/topics/async/#async-safety).
I don't think so, as the worker manages its own connections (independent of Django), and only the user code would use Django connections.
There was a problem hiding this comment.
I was referring to the user code (the task implementation) that is being wrapped here.
What happens then when the user code uses Django connections?
How can they make it work when the thread the code runs in is not the Django main thread?
There was a problem hiding this comment.
From my understanding, it shouldn't matter if the Django connection wasn't initially created in the main thread but directly in the user thread. There is also no request/response cycle like in a Django view that messes around with those connections. And as you already recognized, our previous sync_to_async (with back then thread_sensitive=True) also didn't use the main thread as the Django command never started the worker with async_to_sync. All sync tasks (back then when thread_sensitive was True) used a single thread, but it wasn't the main thread.
Since we switched to thread_sensitive=False all sync tasks use their own separate thread. This should also work as Django connections are thread local. It should work as long as the user doesn't start threads on their own and reuses the connections there (the same as in Django itself).
There was a problem hiding this comment.
Ah yes, as long as we don't try to support thread_sensitive=true, that should be fine.
There was a problem hiding this comment.
Yes, and I also think that using asyncio.to_thread is, in our scenario, the same as using sync_to_async with thread_sensitive=False as we always started our worker in the Django command with asyncio.run and never used async_to_sync there.
medihack
force-pushed
the
replace-asgiref-in-non-django-code
branch
from
f08edc5 to
c8b7d66
Compare
medihack
force-pushed
the
replace-asgiref-in-non-django-code
branch
from
c8b7d66 to
457a762
Compare
|
@ewjoachim, is there anything we still have to do here? |
Closes #1222
Successful PR Checklist:
PR label(s):