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

refactor(core): avoid unwrap, expect, panic in persistence.rs #259

Merged
merged 3 commits into from
Dec 6, 2022

Conversation

dan-da
Copy link

@dan-da dan-da commented Dec 3, 2022

Addresses #247. (more to come)

This PR focuses on persistence.rs. Modifications to other files are just to handle the persistence changes.

persistence.rs:

  • add BlockStorageError enum
  • modify BlockStorage trait to return Result for reads, writes.
  • modify SimpleFileStorage to return Result for reads, writes
  • modify EmptyStorage to return Result for reads, writes
  • log error instead of panic when block write thread hits error
  • minor rustfmt tweaks

node.rs:

  • add NodeError enum (beginning. more variants to come)
  • log errors from BlockStorage read, write
  • minor tweaks in main.rs to handle above changes.

kindelia_core/src/node.rs Outdated Show resolved Hide resolved
kindelia_core/src/node.rs Outdated Show resolved Hide resolved
@dan-da dan-da requested a review from racs4 December 5, 2022 19:14
@dan-da dan-da marked this pull request as draft December 5, 2022 20:24
@dan-da dan-da marked this pull request as ready for review December 5, 2022 20:25
@dan-da
Copy link
Author

dan-da commented Dec 5, 2022

@racs4 I added a kindelia_core::util::FileSystemError. The purpose is to wrap std::io::Error for any filesystem calls, so that path and optionally context can be reported to the user.

Otherwise the user gets "write failed os error(27)" type errors, without even knowing the file. This is a pain point in rust, and it seems like they might address it soon.
see rust-lang/rfcs#2885

The new error type enables SimpleFileStorage to return rich errors cleanly, as path info does not necessarily belong in BlockStorageError.

It should also be useful for other places that presently return IoResult, for example. Basically any code that interacts with filesystem.


Also, I notice that if a review has been requested and not yet performed and then another commit is pushed, CI does not run. And there is no way to request another review.

I found that a workaround is to convert the PR to a draft and then back to a regular PR, at which point checks run again.

persistence:
* add BlockStorageError enum
* modify BlockStorage trait to return Result for reads, writes.
* modify SimpleFileStorage to return Result for reads, writes
* modify EmptyStorage to return Result for reads, writes
* log error instead of panic when block write thread hits error
* minor rustfmt tweaks

node:
* add NodeError enum
* log errors from BlockStorage read, write

bench:
* ignore Result from BlockStorage write
@dan-da dan-da marked this pull request as draft December 5, 2022 21:19
@dan-da dan-da marked this pull request as ready for review December 5, 2022 21:19
Adds an error type for wrapping std::io::Error and providing path
and context metadata.

Initially it is used by SimpleFileStorage but it is intended for use
by any fn that calls std lib file functions with a path.
@dan-da dan-da marked this pull request as draft December 5, 2022 21:44
@dan-da dan-da marked this pull request as ready for review December 5, 2022 21:44
uses FileSystemError in a couple places in SimpleFileStorage that
were missed previously.
@dan-da dan-da marked this pull request as draft December 5, 2022 22:31
@dan-da dan-da marked this pull request as ready for review December 5, 2022 22:32
@racs4
Copy link
Contributor

racs4 commented Dec 5, 2022

I notice that if a review has been requested and not yet performed and then another commit is pushed, CI does not run.

I will try to fix this.

@racs4 racs4 merged commit 626efb4 into kindelia:dev Dec 6, 2022
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