Make saltnado EventListener._handle_event_socket_recv a coroutine (#66177)#69476
Open
dwoz wants to merge 2 commits into
Open
Make saltnado EventListener._handle_event_socket_recv a coroutine (#66177)#69476dwoz wants to merge 2 commits into
dwoz wants to merge 2 commits into
Conversation
The salt-api EventListener registers _handle_event_socket_recv as the on_recv callback for the publish IPC client. Since 3007 the TCP-based publish client's on_recv_handler schedules the callback via asyncio.create_task(callback(msg)). A plain (sync) function returning None makes asyncio.create_task raise "TypeError: a coroutine was expected, got None" (older Python versions reported the equivalent "object NoneType can't be used in 'await' expression"), flooding salt-api / salt-master logs and silently dropping events. Defining _handle_event_socket_recv as an async function lets asyncio.create_task wrap it cleanly. The function body is unchanged -- no await is needed inside it; the change is purely about returning a coroutine to satisfy the transport's contract. A regression test asserts the callback is a coroutine function and that the returned object is awaitable (the operation the TCP publish client performs), so a future drive-by edit cannot turn it back into a sync function without CI catching it. Fixes saltstack#66177
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Makes
salt.netapi.rest_tornado.saltnado.EventListener._handle_event_socket_recvanasynccoroutine so the TCP-based publish IPC client can schedule it withasyncio.create_task(callback(msg))without raisingTypeError.What issues does this PR fix or reference?
Fixes #66177
Previous Behavior
salt-apiandsalt-masterlogs were flooded with one of the following exceptions for every event delivered through the TCP IPC publish path (any timeEventListeneris in use, e.g.salt-apiwithexternal_auth):(Earlier 3007.x reported the equivalent
TypeError: object NoneType can't be used in 'await' expression.)Each error meant the corresponding event was silently dropped, leaving
EventListenerfutures unresolved and salt-api requests hanging until their timeouts.New Behavior
_handle_event_socket_recvis now anasyncfunction.asyncio.create_task(callback(msg))receives a coroutine, the event is dispatched normally, the log spam stops, and salt-api requests complete on time.The body of the function is unchanged - no
awaitis needed inside it; the fix is purely about returning a coroutine so the transport layer'sasyncio.create_taskcontract is satisfied.A regression test (
tests/pytests/unit/netapi/saltnado/test_event_listener.py) asserts the callback is a coroutine function and that the returned object is awaitable, so a future drive-by edit cannot turn it back into a sync function without CI catching it.Merge requirements satisfied?
changelog/66177.fixed.md)tests/pytests/unit/netapi/saltnado/test_event_listener.py)Commits signed with GPG?
No (matches surrounding non-merge commits on this branch).