Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More consistent and safe use of exceptions in do_remove() #518

Closed
qc00 opened this issue Jun 26, 2023 · 1 comment
Closed

More consistent and safe use of exceptions in do_remove() #518

qc00 opened this issue Jun 26, 2023 · 1 comment
Labels
enhancement New feature or request

Comments

@qc00
Copy link
Contributor

qc00 commented Jun 26, 2023

An extension of #447

Currently, do_remove_impl in S3 (and MongoStorage::do_remove) swallows all errors in the batching loop and reports the keys in the erroring requests in a KeyNotFoundException at the end.

This has multiple issues:

  1. If AWS returned THROTTLING or SLOW_DOWN, we should sleep. Otherwise, a subsequent request that might otherwise work will fail
  2. Lumping all errors into one exception may mask exception types that the caller might want to handle, like permission exception
  3. S3 does not error on deleting non-exist objects, so KeyNotFoundException is very misleading
  4. Due to naive batch logic, the exception might contain keys that have been deleted

For example, the second issue can fool the StorageLock::unlock() logic into believing the lock key has been removed, leading to a race condition or deadlock.

I think it's more reliable to:

  • detect keys that really don't exist in the storage, and continue to report those in a KeyNotFoundException at the end
  • for any other kind of errors, throw a different exception immediately

In the latter case, it's the caller's responsibility to retry immediately or leave the data in a sound state for the user to retry.

@qc00 qc00 added the enhancement New feature or request label Jun 26, 2023
@qc00
Copy link
Contributor Author

qc00 commented Jun 26, 2023

A similar issue exists in do_key_exists[_impl]:

Currently, the function returns false for any exception, so any logic using this function for safety may mistake a transient server/network issue for a key not existing.

@poodlewars poodlewars added bug Something isn't working and removed enhancement New feature or request labels Dec 19, 2023
@poodlewars poodlewars added enhancement New feature or request and removed bug Something isn't working labels Dec 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants