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(node): remove more expect calls in node.rs #267

Merged
merged 1 commit into from
Dec 22, 2022

Conversation

dan-da
Copy link

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

Addresses #247 (more to come)

When combined with #265, all except 2 unwrap/expect are removed from node.rs. The remaining 2 expect occurrences are Mutex::lock().expect(), which I consider acceptable (necessary evil) for now.

This PR endeavors to keep the overall logic the same, except that:

  • add_block() failure(s) results in logged error rather than panic. (logged by main init and tasks: do_handle_mined_block and receive_message)
  • handle_request() failures due to inconsistent block data results in logged error rather than panic. (logged by main task receive_request)
  • handle_message() failure due to "bad magic" (wrong network) results in logged error rather than silently failing. (logged by main task receive_message).
  • get_time() is replaced by try_get_time() in a few places, and failure is logged in main() and main task loop is paused with explanatory log message (trying again each time it wakes up)

node.rs

  • add 4 variants to NodeError
  • return Result<_, NodeError> from add_block() and related Node methods.
  • replace some expect calls with NodeError::Inconsistency
  • return NodeError::BadMagic error from ::handle_message()
  • log error in Node::main if receive_message task fails
  • log error in Node::main if receive_request task fails
  • log error in Node::main if handle_mined_block task fails
  • log error and pause tasks in Node::main if try_get_time() fails
  • replace get_time() with try_get_time() where possible

persistence.rs

  • return Result from closure arg to BlockStorage::read_blocks()

util.rs

  • change EpochError visibility: pub(crate) --> pub

node.rs
 * add 4 variants to NodeError
 * return Result<_, NodeError> from add_block() and
   related Node methods.
 * replace some expect calls with NodeError::Inconsistency
 * return NodeError::BadMagic error from ::handle_message()
 * log error in Node::main if receive_message task fails
 * log error in Node::main if receive_request task fails
 * log error in Node::main if handle_mined_block task fails
 * log error and pause tasks in Node::main if try_get_time() fails
 * replace get_time() with try_get_time() where possible

persistence.rs
 * return Result from closure arg to BlockStorage::read_blocks()

util.rs
 * change EpochError visibility: pub(crate) --> pub
@racs4 racs4 requested review from VictorTaelin and racs4 and removed request for VictorTaelin December 21, 2022 18:10
@racs4
Copy link
Contributor

racs4 commented Dec 22, 2022

Very good job. The code looks perfect

all except 2 unwrap/expect are removed from node.rs

But I think there are still more places causing panic like the HashMap accesses made in add_block, that are of the type: self.map[hash]

@dan-da
Copy link
Author

dan-da commented Dec 22, 2022

But I think there are still more places causing panic like the HashMap accesses made in add_block, that are of the type: self.map[hash]

yeah, I've just been going after unwrap/expect until now. I intend to make another pass for array index stuff.

@racs4
Copy link
Contributor

racs4 commented Dec 22, 2022

I intend to make another pass for array index stuff.

That would be in another PR, correct?

@dan-da
Copy link
Author

dan-da commented Dec 22, 2022

yes, in another PR I mean. trying to keep them as small/focused as I can so easier to review.

@dan-da dan-da marked this pull request as draft December 22, 2022 21:07
@dan-da dan-da marked this pull request as ready for review December 22, 2022 21:07
@racs4 racs4 merged commit 36b5861 into kindelia:dev Dec 22, 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