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

error-stack support Debug hooks in no-std contexts #1556

Merged
merged 10 commits into from
Nov 30, 2022
Merged

Conversation

indietyp
Copy link
Member

@indietyp indietyp commented Nov 30, 2022

🌟 What is the purpose of this PR?

While exploring #1399 we found the spin crate, which implements a no-std equivalent for std::sync types, like RwLock based on spin locks. This PR adds a new feature, hooks, which - if enabled - will replace the RwLock with the spin variant.

Note: while we expect a majority of calls to be read-only, there's still a chance of priority inversion, this is why if both std and hooks features are enabled, we will instead use the std RwLock.

Must read for spin locks and their application: https://matklad.github.io/2020/01/02/spinlocks-considered-harmful.html

I consider using a spinlock okay for error-stack hooks, as we expect hooks to only be initialized very, very infrequently, and most operations are reads. A read is simply a fetch_add + fetch_sub without any spinning.

An alternative approach would be using critical-section instead, which acts like a Mutex, but because we mostly only read, we do not need to hold the hooks exclusively, and critical-section is only available on single thread embedded microprocessors.

Note: this increases our test permutation count to 81!

🔗 Related links

🚫 Blocked by

🔍 What does this change?

  • new feature hook

📋 TODO

  • Documentation
  • Changelog

📜 Does this require a change to the docs?

We need to mention the new feature in our table and the consequences.

@github-actions github-actions bot added the area/libs > error-stack Affects the `error-stack` crate (library) label Nov 30, 2022
@codecov
Copy link

codecov bot commented Nov 30, 2022

Codecov Report

Merging #1556 (81c6c68) into main (605777e) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1556      +/-   ##
==========================================
+ Coverage   56.70%   56.74%   +0.04%     
==========================================
  Files         219      219              
  Lines       14629    14644      +15     
  Branches      382      382              
==========================================
+ Hits         8295     8310      +15     
  Misses       6329     6329              
  Partials        5        5              
Flag Coverage Δ
backend-integration-tests 2.70% <ø> (ø)
error-stack 90.89% <100.00%> (+0.09%) ⬆️
unit-tests 1.75% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/libs/error-stack/src/lib.rs 38.46% <ø> (ø)
packages/libs/error-stack/src/fmt/hook.rs 89.74% <100.00%> (+0.13%) ⬆️
packages/libs/error-stack/src/fmt/mod.rs 95.12% <100.00%> (ø)
packages/libs/error-stack/src/hook.rs 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@TimDiekmann TimDiekmann left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

Do we need to adjust the Makefile to also test hooks on insta checks?

One other question, then I'm happy to approve

@@ -22,6 +22,7 @@ anyhow = { version = "1.0.65", default-features = false, optional = true }
eyre = { version = "0.6", default-features = false, optional = true }
owo-colors = { version = "3", default-features = false, optional = true, features = ['supports-colors'] }
serde = { version = "1", default-features = false, optional = true }
spin = { version = "0.9.4", default-features = false, optional = true, features = ['rwlock', 'once'] }
Copy link
Member

Choose a reason for hiding this comment

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

Does 0.9 also work for our use cases?

Copy link
Member Author

Choose a reason for hiding this comment

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

requirement relaxed in 122a119

Copy link
Member

Choose a reason for hiding this comment

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

Great, thanks!

@indietyp
Copy link
Member Author

Thanks! I completely forgot about the snapshot tests, I have adjusted the Makefile and test cfg guard

@indietyp indietyp marked this pull request as ready for review November 30, 2022 18:45
Copy link
Member

@TimDiekmann TimDiekmann left a comment

Choose a reason for hiding this comment

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

Excellent, thank you, and also for providing the link to the blog post.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/libs > error-stack Affects the `error-stack` crate (library)
Development

Successfully merging this pull request may close these issues.

2 participants