Skip to content

Don't rebuild HTTP session with no connections#1020

Open
martindurant wants to merge 1 commit intofsspec:mainfrom
martindurant:no_rebuild
Open

Don't rebuild HTTP session with no connections#1020
martindurant wants to merge 1 commit intofsspec:mainfrom
martindurant:no_rebuild

Conversation

@martindurant
Copy link
Copy Markdown
Member

Fixes #1019

@martindurant
Copy link
Copy Markdown
Member Author

@Koncopd , latest aiobotocore doesn't like that; but I'm not sure why we are getting an unstarted session the second time around.

@martindurant
Copy link
Copy Markdown
Member Author

Given that there isn't much cost to "rebuilding" the http_sessions without any actual live sessions, and that it only happens once, I think it's not worth trying to prob aiobotocore in the right direction here. What do you think, @Koncopd ?

@Koncopd
Copy link
Copy Markdown

Koncopd commented Apr 20, 2026

As I said, my problem is that rebuilding aiobotocore session (not http sessions) erases the credentials I pass. And if I provide a session, it should not happen that I just lose it on set_session.

@Koncopd
Copy link
Copy Markdown

Koncopd commented Apr 20, 2026

Here I mean

self.session = aiobotocore.session.AioSession(**self.kwargs)

@Koncopd
Copy link
Copy Markdown

Koncopd commented Apr 20, 2026

Btw the failing test (test_session_close) looks strange to me, it runs run_program(run) with True and False, but run parameter doesn’t seem to be passed anywhere inside run_program. I assume it just runs the same code, but fails on the second execution only, so the problem is probably due to caching.

@martindurant
Copy link
Copy Markdown
Member Author

it just runs the same code, but fails on the second execution only

Yes, that is exactly the idea. If I remember, the variable is there to allow a breakpoint in the function, where we know which invocation we are on.

@Koncopd
Copy link
Copy Markdown

Koncopd commented Apr 22, 2026

Thank you, i will try to check the problem later.

@lukedyer-uipath
Copy link
Copy Markdown

Hi — came at this from a perf regression we hit downstream (one of our test suites went from 49s to 173s on s3fs 2026.4.0). I put together a small repro that walks through the states the cached-session check can end up in, in case it's useful: https://github.com/lukedyer-uipath/s3fs-pr1002-repro

I worked through the diagnosis and built the linked repro with help from Claude Code, so any of the claims below are worth a sanity check before acting on — flagging that up front.

I might be misreading some of this, so please correct me, but it looks like there are a couple of states beyond the empty-dict case from #1019 that this PR's hsess._sessions and all(...) might not catch — sharing in case they shape the fix here:

_sessions is None. On aiobotocore 3.x, AIOHTTPSession.__aexit__ clears the dict and sets self._sessions = None, so after await client.close() the getattr(...).values() chain raises AttributeError. The repro from #1001 seems to land here on 3.x. With the hsess._sessions and all(...) guard the check would short-circuit cleanly, but I think the cached client (with its now-closed underlying session) would still be returned on the next call, so the failure might just shift downstream — happy to be wrong about that.

Closed inner sessions with _sessions intact. This is the one we're hitting: when client.close() runs on aiobotocore 2.x, or test fixtures (aiomoto in our case) close their managed clients on teardown, the dict is left populated with aiohttp.ClientSession objects whose .closed = True. The all(...) then fires True on every set_session(), so we get a fresh _create_client + botocore.load_service_model (~25ms disk read) per S3 op. cProfile on one of our tests:

                       s3fs 2026.1.0   s3fs 2026.4.0
set_session            50              164
_create_client         8 (84% hit)     164 (0% hit)
load_service_model     9               101 (~4.3s)

I don't have a confident view on the right shape of the fix — .closed is a reasonable proxy when the user has explicitly closed the client, but it's also true in cases where the cached client is still serviceable (the empty-dict case here, and the closed-but-loop-still-fine case above). A stricter signal might help (something that distinguishes "this client can't be used anymore" from "this client has done some work that's been cleaned up"), but I appreciate that's hard to extract without poking at aiohttp internals further than feels comfortable. Both feel like calls only you'd be in a position to make.

For what it's worth, we've sidestepped this downstream by replacing aiomoto with ThreadedMotoServer over real HTTP — no fixture closes inner sessions, so the check stays quiet. We're unblocked either way; just wanted to share what we found in case it's useful.

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.

s3fs>=2026.2.0 always rebuilds the session

3 participants