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

Replace atomics with watch or oneshot channels #2268

Closed
1 of 5 tasks
teor2345 opened this issue Jun 9, 2021 · 4 comments
Closed
1 of 5 tasks

Replace atomics with watch or oneshot channels #2268

teor2345 opened this issue Jun 9, 2021 · 4 comments
Labels
A-rust Area: Updates to Rust code C-cleanup Category: This is a cleanup I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data

Comments

@teor2345
Copy link
Contributor

teor2345 commented Jun 9, 2021

Motivation

Some atomic sizes and atomic operations are not available on some platforms. Others come with a performance penalty on some platforms.

It's also easy to use a memory ordering that's too weak. Future code changes might require a stronger memory ordering. It's hard to test for memory ordering bugs. Some memory ordering bugs can only be discovered on non-x86 platforms. And when they do occur, they can be rare.

Channels take care of all these portability, maintainability, testing, and correctness issues. So in general, Zebra should avoid atomics.

Fixing this issue is a low priority, unless atomic-using code starts failing tests or causing bugs.

Designs

The "Replacing Atomics with Channels" section of the Zebra async Rust RFC:
https://github.com/ZcashFoundation/zebra/edit/main/book/src/dev/rfcs/0011-async-rust-in-zebra.md#replacing-atomics

Tokio channels:
https://docs.rs/tokio/*/tokio/sync

Solution

Alternatives

We could keep using atomics, and accept these portability, maintainability, testing, and correctness risks.

Related Work

#1678 Design and implement graceful shutdown for Zebra

@teor2345 teor2345 added A-rust Area: Updates to Rust code C-cleanup Category: This is a cleanup S-needs-triage Status: A bug report needs triage P-Medium I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data labels Jun 9, 2021
@oxarbitrage
Copy link
Contributor

@teor2345
Copy link
Contributor Author

I've marked this issue as a low priority.

@oxarbitrage
Copy link
Contributor

Ill send a PR.

#2281 - Ended up blocked. For correctness, we need to use of a method only available in tokio 1.

@mpguerra mpguerra removed the S-needs-triage Status: A bug report needs triage label Jun 17, 2021
@teor2345
Copy link
Contributor Author

teor2345 commented Mar 1, 2022

This general cleanup is not required.

@teor2345 teor2345 closed this as completed Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rust Area: Updates to Rust code C-cleanup Category: This is a cleanup I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data
Projects
None yet
Development

No branches or pull requests

3 participants