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

feat: beatree propagates errors during begin_sync #772

Merged
merged 1 commit into from
Feb 7, 2025

Conversation

gabriele-0201
Copy link
Contributor

No description provided.

Copy link
Contributor Author

gabriele-0201 commented Feb 3, 2025

Copy link
Contributor

@pepyakin pepyakin left a comment

Choose a reason for hiding this comment

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

I find this change a tad bit convoluted!

Just consider this. We wrap begin_sync_task in catch_unwind which returns thread::Result. Then we stringify/type-erase it via anyhow. Then we coerce that into std::io::Result with the ErrorKind::Other! In this context, IMO, every use of Other should be scrutinized.

Let's unwind this spaghetti (🥁).

We can pass the panic payload through the channel and then on the receiving side perform panic::resume_unwind(payload).

One thing is that I am not sure what will get printed upon propagating this panic. I am pretty sure the backtrace printed will originate from resume_unwind which is useless and the one in catch_unwind is way more valuable.

nomt/src/beatree/mod.rs Outdated Show resolved Hide resolved
@gabriele-0201 gabriele-0201 force-pushed the gm_handle_errors_beatree_sync branch from 961bf0e to faf1d6b Compare February 7, 2025 10:21
@gabriele-0201 gabriele-0201 changed the base branch from master to gm_add_task_result February 7, 2025 10:22
Copy link
Contributor

@pepyakin pepyakin left a comment

Choose a reason for hiding this comment

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

The new version shapes up way better. Also want to highlight that nomt/src/beatree/allocator/mod.rs got std::io::Result on some of its functions, specifically allocate and grow, just because I tripped on that a couple of times already.

Requires fixing up the tests.

@gabriele-0201 gabriele-0201 force-pushed the gm_handle_errors_beatree_sync branch from faf1d6b to 3f963ce Compare February 7, 2025 10:52
@gabriele-0201 gabriele-0201 force-pushed the gm_handle_errors_beatree_sync branch from 3f963ce to 17033d0 Compare February 7, 2025 10:55
@gabriele-0201
Copy link
Contributor Author

tests fixed!

Copy link
Contributor

pepyakin commented Feb 7, 2025

Merge activity

  • Feb 7, 6:48 AM EST: A user started a stack merge that includes this pull request via Graphite.
  • Feb 7, 6:49 AM EST: Graphite rebased this pull request as part of a merge.
  • Feb 7, 6:51 AM EST: A user merged this pull request with Graphite.

@pepyakin pepyakin changed the base branch from gm_add_task_result to graphite-base/772 February 7, 2025 11:48
@pepyakin pepyakin changed the base branch from graphite-base/772 to master February 7, 2025 11:48
@pepyakin pepyakin force-pushed the gm_handle_errors_beatree_sync branch from 17033d0 to b19d214 Compare February 7, 2025 11:49
@pepyakin pepyakin merged commit a8e9a7b into master Feb 7, 2025
8 checks passed
@pepyakin pepyakin deleted the gm_handle_errors_beatree_sync branch February 7, 2025 11:51
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