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

Dont raise NotFoundError when deleting file #849

Merged
merged 8 commits into from
Nov 20, 2024

Conversation

spuun
Copy link
Member

@spuun spuun commented Nov 16, 2024

WHAT is this pull request doing?

I see no point in raising FileNotFound when trying to delete a file.

One way of triggering the exception:

  1. Publish a bunch of messages to a queue to get some segment files
  2. Start a consumer on that queue
  3. Remove some segments while the consumer is running
  4. Disconnect the consumer
  5. Purge queue

HOW can this pull request be tested?

See steps above.

@spuun spuun requested a review from a team as a code owner November 16, 2024 15:07
Copy link
Member

@snichme snichme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or can we monkey patch stdlid with a delete! or something that delete if exists otherwise just return.
Just see some repetition here

@spuun
Copy link
Member Author

spuun commented Nov 18, 2024

Or can we monkey patch stdlid with a delete! or something that delete if exists otherwise just return. Just see some repetition here

True. It's MFiles in this case, so I can just add a method to it.

@spuun
Copy link
Member Author

spuun commented Nov 18, 2024

That will actaully be much better. Rescuing like I'm doing now means that #close won't be called.

@spuun spuun requested a review from snichme November 18, 2024 08:28
Also, use Crystals abstraction around LibC.unlink
@spuun
Copy link
Member Author

spuun commented Nov 18, 2024

Did another take. I think this is better.

raise File::Error.from_errno("Error deleting file", file: @path) if code < 0
def delete(*, raise_on_missing = true) : self
if raise_on_missing
File.delete(@path)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enlighten me: We go from libc.unlink to File.delete are they the same?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File.delete (public API) is using Crystal::System::File.delete (non-public API) which will use platform dependent calls. On unix it will be LibC.unlink. Now MFile doesn't work on anything else than *nix, but I think it's nice to use public APIs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aha ok, thanks for the explanation

@spuun spuun merged commit 6f80929 into main Nov 20, 2024
22 of 25 checks passed
@spuun spuun deleted the dont-raise-when-deleting-missing-file branch November 20, 2024 10:24
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.

3 participants