Skip to content

Conversation

@btbest
Copy link
Contributor

@btbest btbest commented May 23, 2025

I can't see why blanket-catching all types of network and client-side issues would be a good thing to do here; the git history doesn't explain it, and test_http passes for me locally with this change. Encountering an error during session.get doesn't imply the file doesn't exist, so it generally seems wrong to return False.

I'm running into this issue because I want to implement retrying if the connection drops or times out. With the current implementation, I can't distinguish whether the server actually doesn't have the file vs. the user's mobile data reception being terrible.

@btbest
Copy link
Contributor Author

btbest commented May 23, 2025

Context: ilastik/ilastik#3019 (the last commit has the workaround without upstreaming this fix)

@martindurant
Copy link
Member

Hm, in the case that the URL points to a non-existent server, exists(url) would now cause an exception rather than returning False.
It is possible that we could differentiate between "does not exist" and "other, maybe retriable error". But HTTP is tricky and generally non-standard, so it would need to consider cases like Gateway Error and Not Authenticated. In general, the right choice will depend on the remote you expect to interact with.

@btbest
Copy link
Contributor Author

btbest commented May 26, 2025

In general, the right choice will depend on the remote you expect to interact with.

I'd like to make the right choice, but the current implementation makes that impossible, because it doesn't allow the consumer to distinguish errors like non-existent server, Gateway Error and Not Authenticated from "checked successfully, and the file isn't there".

Even return r.status < 400 is semantically questionable, because 4xx statuses other than 404 and 5xx statuses don't mean that the answer to """Is there a file at the given path""" is "No" (they mean "you can't/aren't allowed to know")...

@btbest btbest force-pushed the expose-connection-errors branch 3 times, most recently from 65f8d2f to 04867d4 Compare May 26, 2025 09:51
@btbest
Copy link
Contributor Author

btbest commented May 26, 2025

I get that who knows what people have implemented relying on this, so put it behind a kwarg to maintain the default behaviour (sorry about the multiple pushes, I thought "strict" would mask a kwarg downstream for a sec).

@btbest btbest force-pushed the expose-connection-errors branch from 203b020 to db97bd1 Compare October 2, 2025 11:29
@btbest
Copy link
Contributor Author

btbest commented Oct 2, 2025

Bump :) I've modified this so that it doesn't change the current behaviour by default, but adds an option to expose errors so that consumers can implement custom handling if they wish.

Regarding parameter naming: I've checked that strict wouldn't shadow any kwarg with the same name on aiohttp.ClientSession, but of course would be happy to change this to anything else you'd prefer.

@martindurant
Copy link
Member

Can you please add a test for this?

@btbest
Copy link
Contributor Author

btbest commented Nov 21, 2025

Good point... I've also added it for the sync implementation while I'm at it.

I've implemented it now with the special treatment for 404, consistent with the other request methods. Depending on how the server behaves with 404s, this can be slightly more or slightly less convenient for fsspec consumers, but as long as exists only returns False for 404, it doesn't limit the consumer's technical options. I'd also be happy to simplify this to just response.raise_for_status, raising all errors including 404 in strict mode if you think that would be more consistent. (In fact for my particular use case, having no special treatment for 404 here would probably be slightly more convenient.)

@btbest
Copy link
Contributor Author

btbest commented Nov 21, 2025

(And squash-merge once you're happy, or I can squash it all if you prefer)

@martindurant martindurant merged commit 25b805d into fsspec:master Nov 21, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants