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(threads): improve panic handling and docs #382

Merged
merged 1 commit into from
Sep 23, 2024

Conversation

elenaf9
Copy link
Collaborator

@elenaf9 elenaf9 commented Aug 19, 2024

Description

Add some missing documentation about possible panics in riot-rs-threads when a function is expected to be called only inside a thread context.
Avoid panics where possible through early returns.

Also adds some documentation for Channel.

Issues/PRs references

Extracted from #381 (comment).

Open Questions

  • @ROMemories you proposed in feat(threads): sort ThreadList by priority #381 (comment) to use NOTE(no-panic)-style comments to document why an unwrap can't panic. I've instead used expect, because we a) don't know that it will never panic (it's the callers responsibility) and b) it prints the comment as part of the panic message. Wdyt?

  • Avoid panics where possible through early returns.

This concerns the functions yield_same and sleep. If called from outside a thread context, the operations are now no-ops, instead of causing panics. Do you agree that we should prefer no-ops over panics as long as it doesn't cause any undefined behavior?

Change checklist

  • I have cleaned up my commit history and squashed fixup commits.
  • I have followed the Coding Conventions.
  • I have performed a self-review of my own code.
  • I have made corresponding changes to the documentation.

Copy link
Collaborator

@ROMemories ROMemories left a comment

Choose a reason for hiding this comment

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

That leaves the documentation of this module in a much better state.
I'm not sure about the expects vs unwraps however ; given these don't depend on runtime data at all, only on the proper usage of the methods, maybe it would make sense to add debug_assert!s with a developer-friendly message, and unwrap otherwise (or maybe even use unreachable_unchecked if that's any useful performance-wise). @kaspar030 wdyt?

src/riot-rs-threads/src/channel.rs Show resolved Hide resolved
src/riot-rs-threads/src/channel.rs Show resolved Hide resolved
src/riot-rs-threads/src/channel.rs Show resolved Hide resolved
src/riot-rs-threads/src/channel.rs Show resolved Hide resolved
src/riot-rs-threads/src/channel.rs Show resolved Hide resolved
src/riot-rs-threads/src/channel.rs Show resolved Hide resolved
src/riot-rs-threads/src/channel.rs Show resolved Hide resolved
src/riot-rs-threads/src/lock.rs Show resolved Hide resolved
src/riot-rs-threads/src/lock.rs Show resolved Hide resolved
@elenaf9
Copy link
Collaborator Author

elenaf9 commented Aug 19, 2024

maybe it would make sense to add debug_assert!s with a developer-friendly message, and unwrap otherwise

What would be the advantage of this compared to using expect?

According to the Rust docs about Converting Errors into Panics:

Of the two, expect is generally preferred since its msg field allows you to convey your intent and assumptions which makes tracking down the source of a panic easier.

@ROMemories
Copy link
Collaborator

ROMemories commented Aug 19, 2024

What would be the advantage of this compared to using expect?

I'm somewhat concerned about the binary size, as these expect() strings end up in the binary. Not saying that the unwrap()s are necessarily smaller (this should ideally be measured), but using unwrap in these cases makes it rather clear that we don't actually care about providing a user-friendly error message in these cases, which could allow us to later optimize these (almost impossible in production) panics for (binary) size.

@kaspar030
Copy link
Collaborator

lgtm!

@elenaf9 elenaf9 added this pull request to the merge queue Sep 23, 2024
Merged via the queue into future-proof-iot:main with commit 1a8d7e0 Sep 23, 2024
26 checks passed
@elenaf9 elenaf9 deleted the threads/panic-docs branch September 23, 2024 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants