dabla commented on code in PR #62922:
URL: https://github.com/apache/airflow/pull/62922#discussion_r4243606821
##########
task-sdk/src/airflow/sdk/execution_time/comms.py:
##########
@@ -321,7 +353,7 @@ async def asend(self, msg: SendMsgType) -> ReceiveMsgType |
None:
async with self._async_lock:
# Acquire the threading lock without blocking the event loop
loop = asyncio.get_running_loop()
- await loop.run_in_executor(None, self._thread_lock.acquire)
+ await self._acquire_thread_lock(loop)
Review Comment:
Confirmed with a real `CommsDecoder` on a socketpair: after cancelling an
`asend` parked on its response, `_thread_lock` was free while that response was
still unread. `asend` now runs the whole request in one worker thread (take the
lock, write, read, release), through the same `_exchange` as `send()`. A
cancelled await abandons only the future: a request not written yet is dropped,
one already written has its response drained before the lock is released. This
replaces the acquire hand-off from the previous round, and the new test in
`test_comms` fails on the old code (195b5d9d87).
It also turned out faster, since a request now makes one trip to a worker
thread instead of two. Measured locally (old `comms.py` vs new, 4000
`GetVariable` round trips against a responder thread, median of 7 interleaved
rounds): 525 to 318 µs per request with one coroutine, 540 to 330 µs with 50
concurrent ones, about 40% less time per request. That makes me think we need
some form of micro-benchmarking for hot paths like the supervisor comms, run
automatically, so a change that degrades them shows up in CI instead of
depending on someone measuring it locally. I will open a separate draft PR to
start that discussion, it does not belong in this one.
Drafted-by: Claude Opus 5.5; reviewed by @dabla before posting
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
