Add async client transport cleanup - #765
Conversation
Alan4506
left a comment
There was a problem hiding this comment.
Thanks @jonathan343! LGTM overall. Just have a non-blocking question:
AIOHTTPClient.close() and AWSCRTHTTPClient.close() release different things. The aiohttp one closes the ClientSession, while the CRT one closes and clears the pooled connections. I understand why: aiohttp only exposes session.close(), and a closed session cannot be reopened; CRT has no session equivalent, and connections are the only thing it can release.
But the problem is: the close() on a generated client now behaves differently per transport. For example, I verified the divergence with the following script:
import asyncio
from smithy_aws_core.identity import EnvironmentCredentialsResolver
from smithy_http.aio.aiohttp import AIOHTTPClient
from smithy_http.aio.crt import AWSCRTHTTPClient
from aws_sdk_apigateway.client import AsyncAPIGatewayClient
from aws_sdk_apigateway.config import Config
from aws_sdk_apigateway.models import GetAccountInput
REGION = "us-east-1"
async def check(label, transport):
client = AsyncAPIGatewayClient(
config=Config(
endpoint_uri=f"https://apigateway.{REGION}.amazonaws.com",
region=REGION,
transport=transport,
aws_credentials_identity_resolver=EnvironmentCredentialsResolver(),
)
)
await client.get_account(GetAccountInput())
print(f"{label}: call before close -> ok")
await client.close()
try:
await client.get_account(GetAccountInput())
print(f"{label}: call after close -> ok (no error raised)")
except Exception as e:
print(f"{label}: call after close -> {type(e).__name__}: {str(e)[:60]}")
async def main():
await check("aiohttp", AIOHTTPClient())
await check("crt ", AWSCRTHTTPClient())
asyncio.run(main())Output:
aiohttp: call before close -> ok
aiohttp: call after close -> SmithyError: Session is closed
crt : call before close -> ok
crt : call after close -> ok (no error raised)
Do you think this divergence is acceptable? Do we need to document the divergence with some comments?
arandito
left a comment
There was a problem hiding this comment.
Overall this PR looks good to me. Left a couple minor comments related to how we preserve config fields when we deepcopy.
Generate close and async context manager methods for clients, and add matching cleanup support to aiohttp and CRT transports. Preserve shared transports when copying operation configs to avoid duplicating sessions and connection pools.
- Generated close() no longer triggers _ensure_setup(), so exiting an
unused client does no credential/config I/O and can't raise; only
closes a transport that was actually set up
- Guard close() with _derive_lock to prevent double-close races
- CRT close() uses return_exceptions=True so one connection failure
doesn't abandon the rest
ad21867 to
3be287a
Compare
|
@Alan4506 Thanks for flagging this. I agree the transport behavior should be consistent, so I updated both implementations to treat Generated clients now reject operations after closure with I added tests covering sends after closure and context-manager re-entry for both transports. |
Overview
This PR adds deterministic resource cleanup to generated clients and HTTP transports. Generated clients can now close transport resources explicitly or through async context managers, while closing an unused client does not trigger lazy config resolution or credential I/O.
Current State
Generated clients do not expose a lifecycle API for their transport. aiohttp sessions and CRT pooled connections can remain open unless callers reach into the configured transport and close it themselves, which can produce
Unclosed client sessionandUnclosed connectorwarnings during shutdown.New Pattern
Generated clients and HTTP transports now support idempotent
close()methods and async context managers. A generated client closes its configured transport after setup, rejects operations and context-manager re-entry after closure, and avoids initializing config or transport resources solely to close an unused client.AIOHTTPClient.close()closes itsClientSession.AWSCRTHTTPClient.close()attempts to close every pooled connection and clears the pool even when an individual connection close fails. Both transports reject sends after closure.If a service models an operation named
Close, the operation is generated asclose_()so it does not collide with the lifecycleclose()method.Clients can also be closed manually:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.