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

asyncio with two interpreter instances #91375

Closed
mbadaire mannequin opened this issue Apr 4, 2022 · 28 comments
Closed

asyncio with two interpreter instances #91375

mbadaire mannequin opened this issue Apr 4, 2022 · 28 comments
Assignees
Labels
3.12 bugs and security fixes topic-asyncio topic-subinterpreters type-bug An unexpected behavior, bug, or error

Comments

@mbadaire
Copy link
Mannequin

mbadaire mannequin commented Apr 4, 2022

BPO 47219
Nosy @asvetlov, @ericsnowcurrently, @1st1

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2022-04-04.19:57:08.003>
labels = ['type-bug', 'expert-asyncio']
title = 'asyncio with two interpreter instances'
updated_at = <Date 2022-04-04.20:38:00.422>
user = 'https://bugs.python.org/mbadaire'

bugs.python.org fields:

activity = <Date 2022-04-04.20:38:00.422>
actor = 'JelleZijlstra'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['asyncio']
creation = <Date 2022-04-04.19:57:08.003>
creator = 'mbadaire'
dependencies = []
files = []
hgrepos = []
issue_num = 47219
keywords = []
message_count = 1.0
messages = ['416694']
nosy_count = 4.0
nosy_names = ['asvetlov', 'eric.snow', 'yselivanov', 'mbadaire']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue47219'
versions = []

@mbadaire
Copy link
Mannequin Author

mbadaire mannequin commented Apr 4, 2022

Hi,
I have an issue when using asyncio and two interpreter instances each launched and used in a seperated thread.
I am getting a asyncio loop for each thread .However asyncio is getting me the same loop because of this code in get_running_loop. Indeed when I have two interpreter, the ts_id would be the same for both my threads and therefore I will get the cached value of the first thread. cached_running_holder being static, it is the same value for all instances of interpreter.
Maybe we should check if we are in the same interpreter or same thread ,.. I am not sure how it could be fixed.

_asynciomodule.c:
get_running_loop(PyObject **loop)
{
    PyObject *rl;

    PyThreadState *ts = _PyThreadState_GET();
    uint64_t ts_id = PyThreadState_GetID(ts);
    if (ts_id == cached_running_holder_tsid && cached_running_holder != NULL) {

If it does not make sense, I have some sample code but it is not just 10 lines.

@pipiche38
Copy link

hello, is there anything we can do to get some attention ?
This is a blocking issue in our project which spawn several instances from a Python embedded framework

Thanks in advance

@ericsnowcurrently
Copy link
Member

It looks like asyncio has not been updated to support use in multiple interpreters -- the module uses a whole bunch of global variables to store state. Until that is dealt with, asyncio cannot be used reliably with multiple interpreters. See PEP 687. @erlend-aasland

Also note that this is unlikely to be considered a bug, so no backports. Likewise it is unlikely to happen for 3.11 since the feature freeze is literally today. Thus the soonest you may see a fix is 3.12 (Fall 2023), unfortunately.

@erlend-aasland
Copy link
Contributor

Yeah, as soon as PEP 687 land, we can start the process of implementing this. I believe I've got a WIP branch lying around already.

@pipiche38
Copy link

Thanks for the update, we will be patient. just a shame not to have back port, but understood the amount of work.
Now, we would be really please to test it when you want .

Potential testing platforms OS: raspian or fedora

@erlend-aasland
Copy link
Contributor

I can push it to my fork next week, so you can play with it.

@kumaraditya303
Copy link
Contributor

One workaround currently is to not use _asyncio speed up module but to use pure python version as that does not has this limitation.

@pipiche38
Copy link

One workaround currently is to not use _asyncio speed up module but to use pure python version as that does not has this limitation.

interesting. How to you prevent using _asyncio ?

@kumaraditya303
Copy link
Contributor

Before importing asyncio add this line:

import sys
sys.modules["_asyncio"] = None

@pipiche38
Copy link

pipiche38 commented May 24, 2022

@kumaraditya303 if ayncio is imported in many module. does it enough to do it only once while importing asyncio for the first time ?

[edit] at least I did it prior the first occurence of import and this looks pretty good .

@kumaraditya303
Copy link
Contributor

Yes it is only required once when asyncio is imported for the first time.

@pipiche38
Copy link

@kumaraditya303 big big big thanks. This is an awesome workaround .
You have unlocked our main issue preventing us to work with multi-instance.

@gvanrossum
Copy link
Member

Yeah, as soon as PEP 687 land, we can start the process of implementing this. I believe I've got a WIP branch lying around already.

Since PEP 687 is accepted, how about it? Seems a good idea to get this out of the way.

@gvanrossum
Copy link
Member

Also, looking at the code a bit more, it looks like PyThreadState_GetID() returns tstate->id. I wonder if, regardless of all the other things we should do, maybe we could make that truly unique, even across interpreters? It looks like it just increments interp->threads.next_unique_id and uses that. It looks like a simple counter. Maybe that counter could be moved to the runtime state, so it's truly unique? @ericsnowcurrently

@erlend-aasland
Copy link
Contributor

Since PEP 687 is accepted, how about it? Seems a good idea to get this out of the way.

Thanks for the ping. I'll try to find time for this in the coming week.

@ericsnowcurrently
Copy link
Member

Maybe that counter could be moved to the runtime state, so it's truly unique?

That would work. We would use the existing _PyRuntimeState.interpreters.mutex to protect "next_unique_id".

An alternative would be to rely on tstate->thread_id instead of tstate->id, since the former is globally unique.

That said, once _asynciomodule.c has been updated, the cached_running_holder_tsid value would be located on the module state, which is always interpreter-specific. So making tstate->id globally unique (or using tstate->thread_id) might not provide much value.

@gvanrossum
Copy link
Member

I feel that the promise in the comment ("unique ID") is violated by the current code. Either the comment should be updated to something like "unique per interpreter" or (IMO better) we should follow the spirit of the comment. Your proposed implementation sounds fine.

@kumaraditya303
Copy link
Contributor

Thanks for the ping. I'll try to find time for this in the coming week.

If you don't mind, please add me as a reviewer.

@gvanrossum
Copy link
Member

@erlend-aasland Did you ever get to this? It reared its head again.

@erlend-aasland
Copy link
Contributor

@erlend-aasland Did you ever get to this? It reared its head again.

Sorry; for various reasons I haven't been able to devote much time to CPython the last weeks. I picked up my work on this last week, though. Currently, the diff stat is ±500 lines, but I expect it to double or tiple by the time I'm done (there'll be some clinic changes that will explode the diff). I also got a ref. leak after converting TaskType to heap type. It's been time-consuming to debug it on my machine; a ./python.exe -m test -R : test_asyncio takes hours (last run took close to 4 hours!).

I'll create a draft PR so anyone interested can join in and help with debugging and progress.

@kumaraditya303 kumaraditya303 added the 3.12 bugs and security fixes label Oct 3, 2022
@kumaraditya303
Copy link
Contributor

kumaraditya303 commented Oct 3, 2022

I'll create a draft PR so anyone interested can join in and help with debugging and progress.

Looking forward to the draft PR. Thanks

It's been time-consuming to debug it on my machine; a ./python.exe -m test -R : test_asyncio takes hours (last run took close to 4 hours!).

Debugging on Linux is much faster so I will be able to debug it faster.

@erlend-aasland
Copy link
Contributor

FTR, here's my WIP branch: https://github.com/erlend-aasland/cpython/tree/isolate-asyncio
Feel free to take over, Kumar!

@kumaraditya303
Copy link
Contributor

Draft PR here #99122

kumaraditya303 added a commit that referenced this issue Nov 29, 2022
…#99122)

Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
@ericsnowcurrently
Copy link
Member

Thanks for working on this, @kumaraditya303!

@erlend-aasland
Copy link
Contributor

Reopening: the future iter object freelist is still a static global, and we forgot to clean up Tools/c-analyzer/cpython/globals-to-fix.tsv.

erlend-aasland added a commit to erlend-aasland/cpython that referenced this issue Jan 23, 2023
@kumaraditya303
Copy link
Contributor

kumaraditya303 commented Jan 24, 2023

Reopening: the future iter object freelist is still a static global, and we forgot to clean up Tools/c-analyzer/cpython/globals-to-fix.tsv.

I left that freelist intentionally as it is not an issue until we have per interpreter GIL. @markshannon has ideas for a global better freelist so hopefully we should be able to remove this freelist entirely.

@erlend-aasland
Copy link
Contributor

I left that freelist intentionally as it is not an issue until we have per interpreter GIL. @markshannon has ideas for a global better freelist so hopefully we should be able to remove this freelist entirely.

I see, thanks for the heads-up.

@gvanrossum
Copy link
Member

I left that freelist intentionally as it is not an issue until we have per interpreter GIL. @markshannon has ideas for a global better freelist so hopefully we should be able to remove this freelist entirely.

Hm. I wouldn't necessarily count on Mark's single freelist idea to be implemented before we get a per-subinterpreter GIL (esp. since the work on mimalloc seems stalled, alas -- Christian Heimes seems to be occupied by other things).

So I think it would behoove us to move fi_freelist and fi_freelist_len to the "per runtime" structure? Or what am I missing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.12 bugs and security fixes topic-asyncio topic-subinterpreters type-bug An unexpected behavior, bug, or error
Projects
Status: Done
Status: Done
Development

No branches or pull requests

6 participants