create new session if refresh is True

[hutch3232]

• Relates to caching of invalid profiles #938
  One observation is that kwargs is an unused argument. Should I remove it? Or do something else to handle it?

[martindurant]

I have a couple of small comments on wording.

On the kwargs: this is probably a leftover from previous code. It could be removed, but let's not do that in this PR.

[martindurant]

This method is actually async, so you would only call it with await within a coroutine, as happens elsewhere in the class. The exact alias is _connect (with the underscore), whereas connect (without the underscore) is the blocking version.

[hutch3232]

Thank you for the clarifications. I haven't directly worked with async before and overlooked that. Let me know if you have additional suggestions for my edits.

[martindurant]

This method is called by any operation on an s3fs instance.

You might call this directly from async code, so that you can assign the client object to a local variable
(^ this is implied in the documentation on async, but should no longer be important for most people).

[martindurant]

I suppose a test would be nice for completeness: call connect(), and assert that the ._s3 and .session attributes have both changed. I wonder if this will cause warnings that those resources weren't closed, or if garbage collection will take care of it.

[hutch3232]

I wonder if this will cause warnings that those resources weren't closed, or if garbage collection will take care of it.

I added a test that s3 and session has changed but I'm not too sure how to test that the resources are closed.

This line seems to be working:

s3fs/s3fs/core.py

Line 557 in 744d9c6

weakref.finalize(self, self.close_session, self.loop, self._s3creator)

I (locally) added a print message in here:

print(f"closing session for {s3}!!!\n")

s3fs/s3fs/core.py

Lines 566 to 567 in 744d9c6

	def close_session(loop, s3):
	if loop is not None and loop.is_running():

And when I run pytest it printed:

closing session for <aiobotocore.session.ClientCreatorContext object at 0x7f2f21b4dd00>!!!

closing session for <aiobotocore.session.ClientCreatorContext object at 0x7f2f22c0d1c0>!!!

[martindurant]

I (locally) added a print message

That will do, thank you.

If they were not getting closed, it would result in warnings at the end of the test run. I did a merge to main, to make sure all tests would run and will check for warnings when done.

[martindurant]

There is one warning of the correct type, but because this is involving zarr, I don't think this PR is implicated. Will merge.

s3fs/tests/test_s3fs.py::test_with_xzarr
  /usr/share/miniconda/envs/test/lib/python3.10/asyncio/base_events.py:674: RuntimeWarning: coroutine 'ClientCreatorContext.__aexit__' was never awaited
    self._ready.clear()