fix(smithy-http): auto-close aiohttp session to avoid unclosed warnings#711
fix(smithy-http): auto-close aiohttp session to avoid unclosed warnings#711Alan4506 wants to merge 1 commit into
Conversation
2e033e1 to
b069050
Compare
| """ | ||
| connector = session.connector | ||
| if connector is not None and not connector.closed: | ||
| connector._close() # type: ignore[attr-defined] |
There was a problem hiding this comment.
Why are you using an internal private method _close()?
There was a problem hiding this comment.
The finalizer runs from weakref.finalize (on GC / interpreter exit), where there may be no running event loop. aiohttp has no public sync close: session.close() is a coroutine, and connector.close() just schedules a task on the loop, so calling it without a running loop gives:
/Users/yuxnchen/sdk/newServiceProject/smithy-python/packages/smithy-http/src/smithy_http/aio/aiohttp.py:169: RuntimeWarning: coroutine 'TCPConnector.close' was never awaited
connector.close() # type: ignore[attr-defined]
RuntimeWarning: Enable tracemalloc to get the object allocation traceback
Unclosed client session
client_session: <aiohttp.client.ClientSession object at 0x10bce6cf0>
Unclosed connector
connections: ['deque([(<aiohttp.client_proto.ResponseHandler object at 0x10bd0e4a0>, 793804.512562458)])']
connector: <aiohttp.connector.TCPConnector object at 0x105fe6e10>
Instead, _close() is a sync method that tears the connections down and sets _closed, so it's the only thing that works from a finalizer.
The pyproject.toml of smithy-http specifies:
I've verifed that _close method exists for all these 3.x versions of aiohttp (3.14.1 for example); Although it's been renamed to _close_immediately on the master branch, the master branch is for 4.x release only. Moreover, aiohttp has never shipped a stable 4.x, and the _close_immediately name has never been in any installable release. Our <4.0 pin also excludes it.
Let me know if you think there is a better way to do so. It is also worth noting that the use of aiohttp is not a final decision and we may switch to CRT - depends on the outcome of the HTTP client design later.
Problem:
The
AIOHTTPClientnever closes itsaiohttp.ClientSession. Because the generated client deep-copies its config (and thus the transport) on every operation call, each call produced a throwaway session copy. On garbage collection these emitUnclosed client session/Unclosed connectorwarnings that print to stderr.Description of changes:
ClientSessionacross__deepcopy__instead of copying it, so that all per-call copies of the transport point to the same session.weakref.finalizehook that closes the underlying connector when the client is garbage collected (or at interpreter exit). The connector is closed synchronously because no event loop may be running at finalization. Closing it also marks the session as closed, so both warnings are suppressed.Testing:
Verified with the generated DynamoDB client running the example below (create table → put item → get item → delete table):
Before:
After:
Also ran the full
smithy-httptest suite: 334 passed, 3 skipped.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.