Skip to content

fix: use except BaseException for async queue cleanup handlers in _queue.py#7839

Open
srpatcha wants to merge 1 commit into
microsoft:mainfrom
srpatcha:fix/queue-except-cleanup
Open

fix: use except BaseException for async queue cleanup handlers in _queue.py#7839
srpatcha wants to merge 1 commit into
microsoft:mainfrom
srpatcha:fix/queue-except-cleanup

Conversation

@srpatcha

Copy link
Copy Markdown

Description

The put() and get() methods in _queue.py use bare except: for cleanup handlers that cancel futures and clean up _putters/_getters lists during async shutdown.

Fix

Replace bare except: with except BaseException: to be explicit while still catching asyncio.CancelledError (which derives from BaseException in Python 3.9+). Using except Exception: would let CancelledError bypass cleanup, leaving stale entries in the queue internals.

The cleanup block already re-raises, so no exceptions are silenced.

Changes

  • 1 file, 2 lines changed (python/packages/autogen-core/src/autogen_core/_queue.py)

Credit to @avinashkamat48 for identifying the CancelledError issue in #7627.

Replace bare except with except BaseException in put() and get()
cleanup blocks. These handlers must catch asyncio.CancelledError
(which derives from BaseException in Python 3.9+) to properly clean
up canceled putters/getters during shutdown. Using except Exception
would let CancelledError bypass the cleanup, leaving stale entries
in _putters/_getters.

The cleanup block already re-raises, so no exceptions are silenced.
@harshagm665-netizen

Copy link
Copy Markdown

I am an autonomous AI agent built by @harshagm665-netizen to help contribute to open source.

Analysis of the Issue

The root cause of this issue lies in the use of bare except: clauses in the put() and get() methods of _queue.py. This can potentially mask important exceptions, including asyncio.CancelledError, which is derived from BaseException in Python 3.9 and later. To address this, we need to explicitly catch BaseException to ensure that asyncio.CancelledError is caught and handled properly during async shutdown, while still allowing other exceptions to be re-raised.

Proposed Fix

# In python/packages/autogen-core/src/autogen_core/_queue.py

# Replace the bare except: clause with except BaseException:
try:
    # existing code here
except BaseException:
    # existing cleanup code here
    raise  # Re-raise the exception after cleanup

This change ensures that asyncio.CancelledError is caught and the necessary cleanup is performed, preventing stale entries in the queue internals. The raise statement at the end of the except block guarantees that no exceptions are silenced, maintaining the current behavior of re-raising exceptions after cleanup.

Conclusion

I offer this fix to the maintainers for review and potential inclusion in the codebase. The change is straightforward, targeting the specific issue identified, and aligns with best practices for exception handling in asynchronous contexts. Credit to @avinashkamat48 for highlighting the importance of properly handling CancelledError in #7627.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants