Usage of `weakref.finalize` causes `RuntimeError: Loop is not running`

[provinzkraut]

import pytest
from adlfs import AzureBlobFileSystem


@pytest.fixture()
def fs():
    return AzureBlobFileSystem(..., asynchronous=True)


@pytest.mark.asyncio
async def test_something(fs: AzureBlobFileSystem):
    pass

A simple setup like this, where the fs is created within an async context (event loop will be set up by pytest-asyncio here), will result in a RuntimeError: Loop is not running during gc.

This is caused by these lines:

adlfs/adlfs/spec.py

Lines 346 to 349 in adb9c53

	weakref.finalize(self, sync, self.loop, close_service_client, self)
	if self.credential is not None:
	weakref.finalize(self, sync, self.loop, close_credential, self)

which obtain a reference to the current event loop, however, since there's no guarantee when finalization on the refs will run, this can also happen after the referenced event loop has been closed.

---

Since all they're doing is closing things, I would like to propose adding a close method to the implementation, which can be manually invoked to do proper cleanup, and a conditional check on the finalizers, so they may not run if the resources have already been closed.

Then, all that's needed in the code to fix the issue is something like

@pytest.fixture()
async def fs():
    fs = AzureBlobFileSystem(..., asynchronous=True)
    yield fs
    await fs.close()

[vladidobro]

Hi,
what are your thoughts on this, @martindurant ?

After many tries, I was completely unable to use adlfs.AzureBlobFileSystem with zarr.storage.FsspecStore, without running into RuntimeError at the end of the script.

The issue seems to be quite clearly that the zarr.core.sync.loop is already closed when the weakref to the filesystem is finalized.

[martindurant]

The issue seems to be quite clearly that the zarr.core.sync.loop is already closed when the weakref to the filesystem is finalized.

Indeed. This isn't necessarily always the case, though: so the finaliser ought to use the event loop if it still exists and is running (that's the "proper" way to shut down async resources). Is the loop is gone/unusable, either attempted shutdown should be skipped, which might cause other warnings, or a sync version used. The latter is usually possible, but I don't know for the case of adlfs. This is what s3fs does:
https://github.com/fsspec/s3fs/blob/main/s3fs/core.py#L587

[vladidobro]

I noticed that there are quite a few places in fsspec where the filesystems are holding onto resources, and it is quite hard to release them. E.g. SFTPFileSystem comes to mind, which doesn't provide any method to close the SSH connection.

Do you think maybe it would be worth it to add an explicit .close() method on AbstractFileSystem (and ._close() for async) that would by default be a noop, to provide a standard way to free resources?

I can put up a simple PR for adlfs that adds the close method as @provinzkraut suggests (and only attempting to close the credentials similarly to s3fs) if you agree @martindurant .

Also, do you have any idea why the error does not happen with asynchronous=False? Fsspec runs its own daemn thread with the loop, so when the weakref is finalized on interpreter shutdown, the async loop is still running in the daemon thread. But for some reason, this is not the case with the zarr dedicated event loop thread, is it shut down in some earlier stage than the fsspec loop?

[martindurant]

A close() method on AbstractFileSystem could be fine; but as you have seen, there are real problems with the async/sync boundary. So probably you would need an async _close()/close() pair so that the calling code retains control. That still leaves the problem of what happens at del time, since we don't know which thread that might happen in.

any idea why the error does not happen with asynchronous=False

Most of these destructors are supposed to work whether within an async context or not (because del can happen before interpreter shutdown); and in any thread. So the general pattern should be, if the loop is available, the actual shutdown should be run with asyncio.run_coroutine_threadsafe .

the async loop is still running in the daemon thread

Not necessarily!

is it shut down in some earlier stage than the fsspec loop

I don't know if there's a guaranteed order to the thread destruction. It is a shame that zarr didn't chose to reuse fsspec's loop...