-
-
Notifications
You must be signed in to change notification settings - Fork 30.8k
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
base: main
Are you sure you want to change the base?
Conversation
kumaraditya303
commented
Jan 2, 2025
•
edited by bedevere-app
bot
Loading
edited by bedevere-app
bot
- Issue: Store current task on the loop in asyncio #128415
There was a problem hiding this 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.", |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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.") |
There was a problem hiding this comment.
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?
raise RuntimeError(f"Cannot enter into task {task!r} while another " | ||
f"task {loop._current_task!r} is being executed.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.") |
raise RuntimeError(f"Leaving task {task!r} does not match " | ||
f"the current task {loop._current_task!r}.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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}.") |
Thank you for raising the issue! Could you please consider an alternative implementation? Thoughts? |
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 |
@kumaraditya303 sorry, I was inaccurate in my comment. |
I see, I have created a new issue #128633 to look more into this. Let's continue the discussion there. |