Asyncify `request_transport` context manager by khsrali · Pull Request #7209 · aiidateam/aiida-core

khsrali · 2026-02-09T08:44:04Z

This avoids two nested loops in transportQ

codecov · 2026-02-09T08:46:34Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.64%. Comparing base (7536ec2) to head (93c81b0).
⚠️ Report is 117 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7209      +/-   ##
==========================================
+ Coverage   79.63%   79.64%   +0.02%     
==========================================
  Files         565      565              
  Lines       43710    43762      +52     
==========================================
+ Hits        34802    34851      +49     
- Misses       8908     8911       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

agoscinski · 2026-02-10T13:12:24Z

+                self._enters += 1
+            else:
+                self.open()
+        self._enters += 1


Isn't this logic simpler?

if self.is_open: self.open() self._enters += 1

Right now you have a logic that considers self._enters aand self.is_open but also assumes that it works the same to work properly.

Okay this is just taken over from somewhere else in transport. Then we do not touch this logic in this PR. Maybe in a subsequent PR but its not that important

This caught my attention as well, would be good to clean up.

Thanks @danielhollas
ok, the logic which was used in three places, is now moved into a set of helper functions.

khsrali · 2026-02-13T09:50:35Z

+            empty_ctx = contextvars.Context()
+            open_callback_handle = empty_ctx.run(self._loop.create_task, do_open())
+            # Note: after making do_open async, we use create_task instead of call_later.
+            # Passing `context` to create_task is only supported in Python 3.11+, so we use
+            # empty_ctx.run() to ensure the task inherits an empty context.
+            # Once the minimum supported Python version is 3.11, this can be simplified to:
+            # open_callback_handle = self._loop.create_task(do_open(), context=contextvars.Context())


@agoscinski regarding your question about call_later:
call_later is only for synchronous callbacks, not coroutines. Since do_open was made async, call_later can't be used with it.

If you think the comment here is verbose, I could reduce to something like

Suggested change

empty_ctx = contextvars.Context()

open_callback_handle = empty_ctx.run(self._loop.create_task, do_open())

# Note: after making do_open async, we use create_task instead of call_later.

# Passing `context` to create_task is only supported in Python 3.11+, so we use

# empty_ctx.run() to ensure the task inherits an empty context.

# Once the minimum supported Python version is 3.11, this can be simplified to:

# open_callback_handle = self._loop.create_task(do_open(), context=contextvars.Context())

empty_ctx = contextvars.Context()

open_callback_handle = empty_ctx.run(self._loop.create_task, do_open())

# self._loop.create_task supports passing a context but only after Python 3.11+

I reduced the comment

danielhollas · 2026-02-13T09:55:19Z

@khsrali would you mind expanding the PR description to explain what this PR is solving? It's very hard to tell just by reading the code, without knowing the details of the current Transport implementation. (Is there an issue open for this?)

khsrali · 2026-02-13T10:25:18Z

Hi @danielhollas
Thanks for looking into this.

This PR, is pre-requisite (and also general improvement!) for the upcoming #7206 (-> get rids of nest_asyncio in favour of python grrenlets)

This PR removes two nested coroutine loops (calling on open() and close() that they internally call on loop.run_untill_complete) inside request_transport context manager.

The request_transport is always used in an async context in our codebase, there's no reason for it to be sync.

danielhollas

Thanks for the context @khsrali

I don't have capacity to do careful review but looks good on brief look.

danielhollas · 2026-02-13T12:24:36Z

+                self._enters += 1
+            else:
+                self.open()
+        self._enters += 1


This caught my attention as well, would be good to clean up.

…/_track_exit helpers

agoscinski

Just one comment. You decide if you want to change it back.

agoscinski · 2026-02-16T09:31:42Z

+        Manages the ``_enters`` reference counter. If the transport is already open
+        (e.g. opened externally), an extra count is added so the final exit won't close it.
+        """
+        need_open = False


I am not entirely sure about decoupling it. Its usage can be now

self._track_enter() await something # another self._track_enter() happens that will also want to open it self.open()

Yes it is internal, but we developers make also mistakes, and you dont save many lines of code by reusing this function.

yeah, vaild point.
I don't know what's the best practise here. Also repeating this code three times, is subject to mistake. 🤔

Ok @agoscinski, I added a clear docstring that how _track_enter should be used.
I also added asyncio.lock for any potential race opening condition --which I believe there's no double opening in engine. But better to be safe than sorry.

khsrali force-pushed the asyncified-transportQ branch from 971e821 to 5b3d4d4 Compare February 9, 2026 10:31

khsrali requested a review from danielhollas February 9, 2026 15:03

agoscinski reviewed Feb 10, 2026

View reviewed changes

khsrali commented Feb 13, 2026

View reviewed changes

khsrali requested a review from agoscinski February 13, 2026 09:54

danielhollas reviewed Feb 13, 2026

View reviewed changes

khsrali added 2 commits February 13, 2026 14:41

Asyncify request_transport context manager to avoid two nested loops

fe4066f

Refactor Transport context manager enter/exit logic into _track_enter…

88bf124

…/_track_exit helpers

khsrali force-pushed the asyncified-transportQ branch from f3967ac to 88bf124 Compare February 13, 2026 13:51

agoscinski approved these changes Feb 16, 2026

View reviewed changes

review applied

93c81b0

khsrali merged commit 41acc86 into aiidateam:main Feb 17, 2026
16 checks passed

khsrali deleted the asyncified-transportQ branch February 17, 2026 10:09

Conversation

khsrali commented Feb 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Feb 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielhollas commented Feb 13, 2026

Uh oh!

khsrali commented Feb 13, 2026

Uh oh!

danielhollas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agoscinski left a comment

Choose a reason for hiding this comment

Uh oh!

agoscinski Feb 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khsrali Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

khsrali commented Feb 9, 2026 •

edited

Loading

codecov Bot commented Feb 9, 2026 •

edited

Loading

agoscinski Feb 16, 2026 •

edited

Loading

khsrali Feb 17, 2026 •

edited

Loading