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

gh-128415: store current task on loop #128416

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kumaraditya303
Copy link
Contributor

@kumaraditya303 kumaraditya303 commented Jan 2, 2025

Copy link
Contributor

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Some nits.

if (item == NULL || item == Py_None) {
// current task is not set
PyErr_Format(PyExc_RuntimeError,
"Leaving task %R does not match the current task %R.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Having "None" in the message could be a bit confusing. Perhaps something like "Leaving task %R does not match the current task (None)" or "Unexpected leaving task %R"?

@@ -3002,7 +3002,9 @@ def done(self):

def test__enter_task(self):
task = mock.Mock()
loop = mock.Mock()
class LoopLike:
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a module-level class would be better?

raise RuntimeError(f"Leaving task {task!r} does not match "
f"the current task {current_task!r}.")
del _current_tasks[loop]
f"the current task.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you align this one?

Comment on lines +1045 to +1046
raise RuntimeError(f"Cannot enter into task {task!r} while another "
f"task {loop._current_task!r} is being executed.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
raise RuntimeError(f"Cannot enter into task {task!r} while another "
f"task {loop._current_task!r} is being executed.")
raise RuntimeError(f"Cannot enter into task {task!r} while another "
f"task {loop._current_task!r} is being executed.")

Comment on lines +1055 to +1056
raise RuntimeError(f"Leaving task {task!r} does not match "
f"the current task {loop._current_task!r}.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
raise RuntimeError(f"Leaving task {task!r} does not match "
f"the current task {loop._current_task!r}.")
raise RuntimeError(f"Leaving task {task!r} does not match "
f"the current task {loop._current_task!r}.")

@asvetlov
Copy link
Contributor

asvetlov commented Jan 2, 2025

Thank you for raising the issue!

Could you please consider an alternative implementation?
I would suggest adding a new class. BaseEventLoop already exists; let's name it _EventLoopHelpers for example.
The _EventLoopHelpers is a C extension class; it has current_tasks, non_asyncio_tasks, eager_tasks, and most notable asyncio_tasks_head. The last is not PyObject * but llist_node; it is not exposed to Python space.
BaseEventLoop is inherited from _EventLoopHelpers if available; otherwise, global objects from asyncio_state are used.
The proposed helper class is storage for placing all loop-scoped states; in the future, some time-critical methods like call_soon/call_later could be implemented in C if we decide to do it.
The proposed helper class is internal now; we can make it public for reuse by uvloop and others, but now it is out of the scope of the issue.
If a loop is not inherited from _EventLoopHelpers, the module-scoped already existing variables are used, as we have already done.

Thoughts?

@kumaraditya303
Copy link
Contributor Author

kumaraditya303 commented Jan 3, 2025

The _EventLoopHelpers is a C extension class; it has current_tasks, non_asyncio_tasks, eager_tasks, and most notable asyncio_tasks_head. The last is not PyObject * but llist_node; it is not exposed to Python space.

That would work for things like the storing all the tasks and the eager tasks, but for current task one loop will only ever have one current task so all that isn't needed. Specifically I am saying the even for the loop scoped state we don't need current_tasks dict, a single attribute on loop works.

@asvetlov
Copy link
Contributor

asvetlov commented Jan 3, 2025

@kumaraditya303 sorry, I was inaccurate in my comment.
Yes, the only current task is needed instead of the dict after storing the current task in the loop.
I'm talking about a slightly different thing.
We have several loop-scoped global variables now. Some are Python objects, but llist_node is a plain C structure.
Let's make a super-base C Extension class where we can put all these objects (both python and plain C) and inherit BaseEventLoop from it.
I personally feel that we will have more loop-scoped things like llist_node in the future; now is a good time to settle a place for them.

@kumaraditya303
Copy link
Contributor Author

Yes, the only current task is needed instead of the dict after storing the current task in the loop.
I'm talking about a slightly different thing.
We have several loop-scoped global variables now. Some are Python objects, but llist_node is a plain C structure.
Let's make a super-base C Extension class where we can put all these objects (both python and plain C) and inherit BaseEventLoop from it.
I personally feel that we will have more loop-scoped things like llist_node in the future; now is a good time to settle a place for them.

I see, I have created a new issue #128633 to look more into this. Let's continue the discussion there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants