search

duna-oss / flystorage

Flystorage; File storage abstraction for Node / TypeScript
https://flystorage.dev/
172 stars 10 forks source link

Error handling for expected Vs unexpected failures #50

Open davidnx opened 3 weeks ago

davidnx commented 3 weeks ago

The current api makes no attempt (as far as I can tell) to distinguish deterministic expected failure modes (e.g. attempting to read a file that does not exist) from others (e.g. transient network errors).

For example, FileStorage.read will simply throw an UnableToReadFile error regardless of what caused it.

https://github.com/duna-oss/flystorage/blob/91b53f337188871d845b1c2ced474df96eac17bd/packages/file-storage/src/file-storage.ts#L189-L198

It would be appropriate to introduce error codes so that consuming code can decide how to handle different conditions, and various implementations of StorageAdapter would have to abide by the rules and transform upstream errors appropriately. While one could call FileStorage.fileExists first to check if a file exists, that is insufficient in the general case, as a file could be deleted in the backing storage between the call to fileExists and the subsequent read.

I really like the simplistic api that flystorage provides, and tackling this would make it appealing in even more applications. Thanks!

frankdejonge commented 3 weeks ago

Thank you for the suggestion, I think this would indeed make the errors a little bit more useful. In order for that to really be easy, we'd also need a utility to check which scenario the error is for, since errors can be anything.

frankdejonge commented 3 weeks ago

Might also make sense to share the context as to why this wasn't designed like that in the first place. Primarily, especially with two calls, there's a race-condition problem, a performance penalty, and an intermittent unavailability problem. When you first check the existence and then read, the file may be deleted between the check and the read. Doing the call prior to the read means reads are then, per definition, two reads. Doing it after the call makes errors more expensive. When there are network issues, they're often not stable, which may cause one of these calls to be OK but the other to fail, which gives non-deterministic outcomes.

davidnx commented 2 weeks ago

+1.

[Frank] Might also make sense to share the context as to why this wasn't designed like that in the first place.

@frankdejonge who was this comment meant for? As far as I can tell you are the original author, let me know how I can help here.

frankdejonge commented 2 weeks ago

@davidnx the comment was mean for you, I wanted to highlight why it is the way it currently is so when/if it changes we know what benefits not having it has. We'll know what we're sacrificing. It's a trade-off design decision.

davidnx commented 2 weeks ago

Right, I'm right there with you re. the problem with the current design, hence why I filed this issue.

You explained the exact problem that this issue is about:

When you first check the existence and then read, the file may be deleted between the check and the read. Doing the call prior to the read means reads are then, per definition, two reads. Doing it after the call makes errors more expensive. When there are network issues, they're often not stable, which may cause one of these calls to be OK but the other to fail, which gives non-deterministic outcomes.

The current design forces consumers to do the bad thing (two reads), because there is no way to unambiguously determine the kind of failure on each read.

I also alluded to this in the issue description:

While one could call FileStorage.fileExists first to check if a file exists, that is insufficient in the general case, as a file could be deleted in the backing storage between the call to fileExists and the subsequent read.

The api needs to be redesigned / the Error classes have to be modified, so that unambiguous error codes can be observed by the callers.