Initialisation seems to maintain cached filesystem

[benrutter]

I'm not 100% sure that the title here, is accurate, as it involves a bit more understanding of what's happening under the hood with asyncssh than I have so far.

I'm also not sure if this is intended behaviour vs actually a bug (sorry!)

The issue is something like this:

fs = SSHFileSystem(host, username=username, password=password)
for filepath in long_list_of_files:
    with fs.open(filepath) as file:
        _ = file.read()

If this runs for a long time, the connection might be shut down from the other side throwing up an asnycssh.sftp.SFTPNoConnection error, so far this is all as expected.

The bit that seems unusual is that something like this:

fs = SSHFileSystem(host, username=username, password=password)
for filepath in long_list_of_files:
    try:
        with fs.open(filepath) as file:
            _ = file.read()
    except SFTPNoConnection:
        new_fs = SSHFileSystem(host, username=username, password=password)
        with new_fs.open(filepath) as file:
            _ = file.read()

The new_fs will always throw up the same SFTPNoConnection error, which seems to be because something behind the scenes is being cached?

Notably, the following works by clearing the cache before reconnecting:

fs = SSHFileSystem(host, username=username, password=password)
for filepath in long_list_of_files:
    try:
        with fs.open(filepath) as file:
            _ = file.read()
    except SFTPNoConnection:
        fs.clear_instance_cache()
        new_fs = SSHFileSystem(host, username=username, password=password)
        with new_fs.open(filepath) as file:
            _ = file.read()

I'd assume the expected behaviour would be that initialising a new SSHFileSystem would create a fully new connection - is this intentional behaviour?

[benrutter]

Should also mentioned that there's a stack overflow issue related to this

[benrutter]

I've done some playing around and noticed the connection isn't closed during finalize is that intentional? Also, is there any reason not to put a self.clear_instance_cache() in that finalize step? (I feel like there probably is, I'm mainly just interested!)

[fkrauthan]

We discovered the same issue. Because of the cached file system this library actually leaks connections as every time you initialize an instance of SSHFileSystem it connects to the SFTP server while all method calls use the very first instance only.

Given that the library opens a new connection as part of the constructor it probably should overwrite the cachable attribute of AbstractFileSystem and set it to False.

A workaround until that is done:

class SSHFileSystemNoCache(SSHFileSystem):
    cachable = False

and then use for your code the SSHFileSystemNoCache class instead.

Also it would be great to explicit be able to close the connection. Currently I use a sf.client.close() to enforce that after I am done using my fs instance.

[shcheklein]

Closing with the resolution options — sshfs already has the knobs for every variant of this today:

• Stale idle connections (the case described here): pass keepalive_interval — an asyncssh option forwarded by sshfs — e.g. SSHFileSystem(host, keepalive_interval=30). The client-side pings keep the connection alive through idle periods instead of the cache silently handing back a dead one.
• Fresh connection on demand: SSHFileSystem(..., skip_instance_cache=True) bypasses fsspec's instance cache; SSHFileSystem.clear_instance_cache() resets it in long-lived processes.
• Connection leaks (@fkrauthan's report): that pattern usually means constructor arguments that tokenize differently on each call (callbacks, freshly-built objects) — each variant becomes its own cached instance holding a live connection, and the cache's strong references keep the finalizers from ever running. Stable arguments or skip_instance_cache=True avoid it.

Disabling the instance cache wholesale (#60) isn't the right default — every construction would pay a full SSH handshake, a heavy cost for DVC/fsspec workflows; see the PR for the details, including why the patch as written was a no-op.

If there's appetite for defaulting keepalive_interval to a sane value out of the box, that's a focused change we'd gladly take a PR for — please open a fresh issue for it.