Fix FsspecFileIO.get_fs thread safety#2495
Merged
Fokko merged 1 commit intoapache:mainfrom Sep 22, 2025
Merged
Conversation
AlexWendland
approved these changes
Sep 22, 2025
AlexWendland
left a comment
AlexWendland
left a comment
There was a problem hiding this comment.
The use of the local self._thread_locals variable ensure the lru_cache is distinct to each thread. This enables per-thread connection pooling of HTTP requests. This has been correctly removed and added back upon serialisation/deserialisation.
lwfitzgerald
force-pushed
the
fix/fsspec-cache-thread-safety
branch
4 times, most recently
from
0450687 to
308dee2
Compare
kevinjqliu
approved these changes
Sep 22, 2025
Contributor
kevinjqliu
left a comment
kevinjqliu
left a comment
There was a problem hiding this comment.
LGTM Thanks for the detailed PR + test :)
`FsspecFileIO.get_fs` can be called by multiple threads when `ExecutorFactory` is used (for example by `DataScan.plan_files`). The base class of `fsspec` filesystem objects, `fsspec.spec.AbstractFileSystem`, internally caches instances through the `fsspec.spec._Cached` metaclass. The caching key used includes `threading.get_ident()`, making entries thread-local: https://github.com/fsspec/filesystem_spec/blame/f84b99f0d1f079f990db1a219b74df66ab3e7160/fsspec/spec.py#L71 The `FsspecFileIO.get_fs` LRU cache (around `FsspecFileIO._get_fs`) breaks the thread-locality of the filesystem instances as it will return the same instance for different threads. One consequence of this is that for `s3fs.S3FileSystem`, HTTP connection pooling no longer occurs per thread (as is normal with `aiobotocore`), as the `aiobotocore` client object (containing the `aiohttp.ClientSession`) is stored on the `s3fs.S3FileSystem`. This change addresses this by making the `FsspecFileIO.get_fs` cache thread-local.
lwfitzgerald
force-pushed
the
fix/fsspec-cache-thread-safety
branch
from
308dee2 to
9d2354d
Compare
Contributor
Author
|
Removed some unnecessary events ^ |
lwfitzgerald
added a commit
to man-group/iceberg-python
that referenced
this pull request
Sep 22, 2025
The existing S3 remote signing hook function (`s3v4_rest_signer`) uses `requests.post` to submit `POST` requests to the REST signing endpoint, but this internally creates a new `requests.Session` for every request, preventing any reuse of connections. In my profiling I saw this add overhead from repeated loading of CA certs and reestablishing of TLS connections. This change makes the signing function a callable object that wraps a `request.Session`, using this for `POST`ing, therefore achieving connection reuse. Signer callables are stored on the hook internals of the `aiobotocore` client inside the `s3fs.S3FileSystem` instance, so use and lifetime will match that of those instances. The `s3fs.S3FileSystem` instances are guaranteed thread-local since: apache#2495.
Fokko
approved these changes
Sep 22, 2025
Contributor
Fokko
left a comment
Fokko
left a comment
There was a problem hiding this comment.
Thanks @lwfitzgerald for working on this 🙌 This looks great!
lwfitzgerald
added a commit
to man-group/iceberg-python
that referenced
this pull request
Sep 25, 2025
The existing S3 remote signing hook function (`s3v4_rest_signer`) uses `requests.post` to submit `POST` requests to the REST signing endpoint. This internally creates a new `requests.Session` for every request, preventing any reuse of connections. In my profiling I saw this add overhead from repeated loading of CA certs and reestablishing of TLS connections. This change makes the signing function a callable object that wraps a `request.Session`, using this for `POST`ing, therefore achieving connection reuse. Signer callables are stored on the hook internals of the `aiobotocore` client inside the `s3fs.S3FileSystem` instance, so use and lifetime will match that of those instances. They are thread-local since: apache#2495.
lwfitzgerald
added a commit
to man-group/iceberg-python
that referenced
this pull request
Sep 25, 2025
The existing S3 remote signing hook function (`s3v4_rest_signer`) uses `requests.post` to submit `POST` requests to the REST signing endpoint. This internally creates a new `requests.Session` for every request, preventing any reuse of connections. In my profiling I saw this add overhead from repeated loading of CA certs and reestablishing of TLS connections. This change makes the signing function a callable object that wraps a `request.Session`, using this for `POST`ing, therefore achieving connection reuse. Signer callables are stored on the hook internals of the `aiobotocore` client inside the `s3fs.S3FileSystem` instance, so use and lifetime will match that of those instances. They are thread-local since: apache#2495.
Fokko
pushed a commit
that referenced
this pull request
Oct 1, 2025
# Rationale for this change The existing S3 remote signing hook function (`s3v4_rest_signer`) uses `requests.post` to submit `POST` requests to the REST signing endpoint. This internally creates a new `requests.Session` for every request, preventing any reuse of connections. In my profiling I saw this add overhead from repeated loading of CA certs and reestablishing of TLS connections. This change makes the signing function a callable object that wraps a `request.Session`, using this for `POST`ing, therefore achieving connection reuse. Signer callables are stored on the hook internals of the `aiobotocore` client inside the `s3fs.S3FileSystem` instance, so use and lifetime will match that of those instances. They are thread-local since: #2495. ## Are these changes tested? Tested locally. Existing unit tests updated for changes. ## Are there any user-facing changes? Yes - S3 signing requests now use connection pools (scoped to the `s3fs.S3FileSystem` object).
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
4 participants
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.
Rationale for this change
FsspecFileIO.get_fscan be called by multiple threads whenExecutorFactoryis used (for example byDataScan.plan_files).The base class of
fsspecfilesystem objects,fsspec.spec.AbstractFileSystem, internally caches instances through thefsspec.spec._Cachedmetaclass. The caching key used includesthreading.get_ident(), making entries thread-local:https://github.com/fsspec/filesystem_spec/blob/f84b99f0d1f079f990db1a219b74df66ab3e7160/fsspec/spec.py#L71
The
FsspecFileIO.get_fsLRU cache (aroundFsspecFileIO._get_fs) breaks the thread-locality of the filesystem instances as it will return the same instance for different threads.One consequence of this is that for
s3fs.S3FileSystem, HTTP connection pooling no longer occurs per thread (as is normal withaiobotocore), as theaiobotocoreclient object (containing theaiohttp.ClientSession) is stored on thes3fs.S3FileSystem.This change addresses this by making the
FsspecFileIO.get_fscache thread-local.Are these changes tested?
Tested locally. Unit test included covering the caching behaviour.
Are there any user-facing changes?
Yes - S3 HTTP connection pooling now occurs per-thread, matching the behaviour of
aiobotocorewhen it used in the recommended way with an event loop per thread.