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.

Sign up for GitHub

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

Jump to bottom

Is Sync for Iter necessary? #636

Closed
kornelski opened this issue May 8, 2021 · 2 comments · Fixed by #640
Closed

Is Sync for Iter necessary? #636

kornelski opened this issue May 8, 2021 · 2 comments · Fixed by #640
Labels
A-tonic E-easy Call for participation: Experience needed to fix: Easy / not much E-help-wanted Call for participation: Help is requested to fix this issue.
Milestone
0.5

Comments

@kornelski
Copy link

tonic/tonic/src/metadata/map.rs

Line 1247 in 4a917a3

unsafe impl<'a> Sync for Iter<'a> {}

http::header::Iter implements Sync if the element is Sync, and HeaderValue is Sync. So this unsafe impl seems unnecessary. I suggest removing it just in case, because if any future change made anything not actually Sync, it could end up incorrectly overriding it.
@davidpdrsn davidpdrsn added A-tonic I-needs-decision Issues in need of decision. labels May 8, 2021
@davidpdrsn
Copy link
Member

Yeah looks like that could be removed. Same goes for IterMut.

I'm not familiar with that part of the code so would like to hear what @LucioFranco thinks.

@LucioFranco
Copy link
Member

Yeah, we could probably remove it but we should make sure it continues to be Sync/Send via a test.

@davidpdrsn davidpdrsn added E-easy Call for participation: Experience needed to fix: Easy / not much E-help-wanted Call for participation: Help is requested to fix this issue. and removed I-needs-decision Issues in need of decision. labels May 8, 2021
@davidpdrsn davidpdrsn added this to the 0.5 milestone May 12, 2021
davidpdrsn added a commit that referenced this issue May 12, 2021
@davidpdrsn
feat(metadata): remove manual Send + Sync impls for metadata types
a1850f8 
Removes manual `Send` and `Sync` impls for these types as they weren't
necessary:

- `metadata::Iter`
- `metadata::IterMut`
- `metadata::ValueDrain`
- `metadata::ValueIterMut`

Since all the types they contained were `Send + Sync` we can rely on the
compiler automatically adding the impls.

Also added some tests to ensure we don't accidentally make them `!Send`
or `!Sync` in the future.

Fixes #636
@davidpdrsn davidpdrsn mentioned this issue May 12, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-tonic E-easy Call for participation: Experience needed to fix: Easy / not much E-help-wanted Call for participation: Help is requested to fix this issue.
Projects
None yet
Milestone
0.5
Development

Successfully merging a pull request may close this issue.

feat(metadata): remove manual Send + Sync impls for metadata types hyperium/tonic
3 participants
@kornelski @davidpdrsn @LucioFranco