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.

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

Fix for current_message while operating under AsyncIO #593

Merged
merged 3 commits into from Dec 17, 2023

Conversation

pahrohfit
Copy link
Contributor

Moved from locals to contextvars to address issues while running under AsyncIO.

Fixes #586

@Bogdanp
Copy link
Owner

Bogdanp commented Dec 11, 2023

This looks good, thanks! One question, thouhg, because I haven't been keeping up with contextvars. Do you know if they play well with gevent?

@spumer
Copy link

spumer commented Dec 11, 2023

They play well with gevent, but you need pass it to greenlet.gr_context attribute

https://greenlet.readthedocs.io/en/latest/contextvars.html

@pahrohfit
Copy link
Contributor Author

They play well with gevent, but you need pass it to greenlet.gr_context attribute

https://greenlet.readthedocs.io/en/latest/contextvars.html

Yeah, I read this, didn't dig crazy into the dramatiq implementation, but the unit tests appear fully functional (and the unit test as written does indeed properly test the functionality):

❮ python pytest-gevent.py -x tests/test_actors.py
/Users/rob/noid/dramatiq-venv/lib/python3.11/site-packages/pytest_benchmark/logger.py:46: PytestBenchmarkWarning: Can't compare. No benchmark files in '/Users/rob/noid/dramatiq/.benchmarks'. Can't load the previous benchmark.
  warner(PytestBenchmarkWarning(text))
======================================================================================= test session starts ========================================================================================
platform darwin -- Python 3.11.6, pytest-7.4.3, pluggy-1.3.0
benchmark: 4.0.0 (defaults: timer=time.perf_counter disable_gc=False min_rounds=5 min_time=0.000005 max_time=1.0 calibration_precision=10 warmup=False warmup_iterations=100000)
rootdir: /Users/rob/noid/dramatiq
configfile: setup.cfg
plugins: cov-4.1.0, benchmark-4.0.0
collected 25 items

tests/test_actors.py .........................                                                                                                                                               [100%]/Users/rob/noid/dramatiq-venv/lib/python3.11/site-packages/pytest_benchmark/logger.py:46: PytestBenchmarkWarning: Not saving anything, no benchmarks have been run!
  warner(PytestBenchmarkWarning(text))


---------- coverage: platform darwin, python 3.11.6-final-0 ----------
Coverage HTML written to dir htmlcov


======================================================================================== 25 passed in 7.64s ========================================================================================

Full disclosure, I did have to modify the tox.ini to run on current versions of tox (s/-cpython//g and s/whitelist_externals/allowlist_externals/g) -- but that should not have impacted any of the testing. @Bogdanp if you want me to include that updated tox.ini, let me know.

@spumer
Copy link

spumer commented Dec 11, 2023

Looks like async_to_sync do not copyback contextvars.

New test check contextvars copy into asyncio thread, but not back. I think copyback contextvars is needed cause async_to_sync used as transparent gateway and other user middlewares can fail with asyncio if they uses contextvars.

It's not for that PR, but who knows :)

@pahrohfit
Copy link
Contributor Author

Looks like async_to_sync do not copyback contextvars.

New test check contextvars copy into asyncio thread, but not back. I think copyback contextvars is needed cause async_to_sync used as transparent gateway and other user middlewares can fail with asyncio if they uses contextvars.

It's not for that PR, but who knows :)

You mean to have a thread modify an original contextvar from the caller, outside of what CurrentMessage() is doing?

@spumer
Copy link

spumer commented Dec 12, 2023

You mean to have a thread modify an original contextvar from the caller, outside of what CurrentMessage() is doing?

Yes, if async function running in loop (in asyncio thread) modify some contextvars this changes does not reflects in caller thread, but for sync function it does.

Right now we fix situation when changes in caller thread not reflected in asyncio thread, but we have other side - changes in asyncio should reflect caller thread.

Of course i'm talking only about contextvars, we are not interesting in any other changes

@pahrohfit
Copy link
Contributor Author

pahrohfit commented Dec 12, 2023

You mean to have a thread modify an original contextvar from the caller, outside of what CurrentMessage() is doing?

Yes, if async function running in loop (in asyncio thread) modify some contextvars this changes does not reflects in caller thread, but for sync function it does.

Right now we fix situation when changes in caller thread not reflected in asyncio thread, but we have other side - changes in asyncio should reflect caller thread.

Of course i'm talking only about contextvars, we are not interesting in any other changes

Gotcha -- but that wouldn't impact the CurrentMessage() middleware, since that is currently geared exclusively at a worker obtaining its own messages -- not passing things back up that pipeline:

With this middleware in place, every actor can access its own message by calling get_current_message on the CurrentMessage class

This sounds like net new direct functionality (versus an artifact of how the locals() is working today?), like a state instance (ala ctx in arq.worker.Worker())?

@pahrohfit
Copy link
Contributor Author

Hmm ... looks like a problem with the rabbitmq server in the CI for those unit tests that failed

@Bogdanp Bogdanp merged commit 456aa1c into Bogdanp:master Dec 17, 2023
1 of 11 checks passed
@Bogdanp
Copy link
Owner

Bogdanp commented Dec 17, 2023

Thanks!

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.

CurrentMessage don't work with AsyncIO
3 participants