Skip to content
This repository has been archived by the owner on May 3, 2021. It is now read-only.

Noop locking #50

Merged
merged 4 commits into from
Dec 28, 2019
Merged

Noop locking #50

merged 4 commits into from
Dec 28, 2019

Conversation

furuholm
Copy link
Contributor

This PR implements the noop locking part of #49.

@furuholm
Copy link
Contributor Author

furuholm commented Dec 25, 2019

I added a static assertion as suggested in #49 but I am not sure I did it correctly. First of all the docs says that the following is to be added to lib.rs

#[macro_use]
extern crate static_assertions;

This lead to a unused `#[macro_use]` import warning so I didn't include those lines (the macro still worked).

I then verified that static_assertions::assert_not_impl_all!(Context: Send, Sync); worked by adding Send and Sync implementations for Context. As expected this caused a compilation error but the error message not very clarifying.

type annotations needed for `fn() {<context::Context as context::_::{{closure}}#0::AmbiguousIfImpl<_>>::some_item}

This may very well be correct, but I wasn't sure so it would be great if you could have an extra look at this.

Also: Travis failed. Not sure why.

$ cargo test --all --verbose --no-default-features --features="crypto-openssl"
error: --features is not allowed in the root of a virtual workspace
The command "cargo test --all --verbose --no-default-features --features="crypto-openssl"" exited with 101.

Not sure what this is about. The same command worked in the master build.

@furuholm
Copy link
Contributor Author

I think the build failure is caused by rust-lang/cargo#7507. I will try to fix this in a separate PR in the coming days.

Copy link
Contributor

@Michael-F-Bryan Michael-F-Bryan left a comment

Choose a reason for hiding this comment

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

Looks good to me. Other than my comment about moving the static assert next to the code that relies on the assertion, I'd be happy to merge this.

//
// See https://github.com/Michael-F-Bryan/libsignal-protocol-rs/issues/49
// for details.
static_assertions::assert_not_impl_all!(Context: Send, Sync);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this line be moved directly above lock_function()? That way our assertion that Context: !Send + !Sync is right next to the comment that says locking isn't required as long as Context doesn't implement Send and Sync.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. This looks better.

Fixed in eddb1cd

@furuholm
Copy link
Contributor Author

The build failure is fixed by #52. I would like to have that merged before this change to not fail the build on master.

@Michael-F-Bryan
Copy link
Contributor

@furuholm I've merged #52. If you do a rebase we should be good to merge.

This change removes the mutex capabilities of lock_function
and unlock_function as it turns out that locking is not required
as `Context` cannot be shared between threads as long as it
does not implement the `std::marker::Sync` trait.
@furuholm
Copy link
Contributor Author

@Michael-F-Bryan rebase done...

@furuholm
Copy link
Contributor Author

Build still fails for Rust: 1.34.0 though. Seems that the static_assertions lib does not compile.

@Michael-F-Bryan
Copy link
Contributor

Looking at the .travis.yml for nvzqz/static-assertions-rs I think we need to bump the minimum rustc version to 1.37.0.

Would you be able to update this line with the new version and also add a comment saying it's needed for static_assertions?

The static_assertions crate requires rustc 1.37.0
@Michael-F-Bryan Michael-F-Bryan merged commit efc99af into whisperfish:master Dec 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants