fsspec / filesystem_spec

A specification that python filesystems should adhere to.
BSD 3-Clause "New" or "Revised" License
1.02k stars 358 forks source link

Have consistent behavior when asyn.sync_wrapper decorated method have timeout #644

Closed scott-zhou closed 3 years ago

scott-zhou commented 3 years ago

asyn.sync_wrapper can convert an async method to a sync method. When the user calls the decorated sync method, it accepts a timeout argument to specify the maximum execution time.

When the timeout happen, it can be two different behaviors:

This is a bit confusing and ambiguous result. For the first case, the user should not care about the fsspec's implementation detail, so it should not expect an asyncio.TimeoutError. And for the second case, the user doesn't know if the function actually succeeds but returns a None result, or it is timeout.

I think fsspec should have it's own user-defined exception FSTimeoutError and use that exception on either case of timeout.

martindurant commented 3 years ago

When the user calls the decorated sync method, it accepts a timeout argument to specify the maximum execution time.

Right you are! I had totally forgotten I had implemented this. Returning None is totally the wrong thing to do :)

I agree with your conclusion, and that a consistent exception should be raise, but I have no string opinion on which. If a custom exception, it should derive from asyncio.TimeoutError

martindurant commented 3 years ago

Note that asyncio.TimeoutError is a subclass of concurrent.futures.TimeoutError

scott-zhou commented 3 years ago

Aha, no one can remember everything, it might be a bit long time ago :) I have changed the PR as you mentioned, the FSTimeoutError inheritance from asyncio.TimeoutError