Fix CI failures #642
Fix CI failures #642
Conversation
There's no need for this pub use, as the module doesn't contain any public types or functions. Signed-off-by: Andrew Walbran <qwandor@google.com>
Signed-off-by: Andrew Walbran <qwandor@google.com>
Signed-off-by: Andrew Walbran <qwandor@google.com>
|
If anyone knows how to fix the Windows issues suggestions are welcome, otherwise can we at least get this merged to fix the rest? |
| @@ -10,4 +10,3 @@ mod bindings { | |||
| mod grpc_wrap; | |||
|
|
|||
| pub use bindings::*; | |||
| pub use grpc_wrap::*; | |||
There was a problem hiding this comment.
It generates an "unused import" warning building with the nightly toolchain, which fails CI. The grpc_wrap module has no public members, so this use statement is actually a no-op. (grpc_wrap does add implementations to other types, but they don't need importing as they aren't part of its namespace.)
| @@ -22,7 +22,6 @@ mod protobuf_v3 { | |||
| #[cfg(feature = "protobuf-codec")] | |||
| mod reexports { | |||
| pub use super::protobuf::health::*; | |||
| pub use super::protobuf::health_grpc::*; | |||
There was a problem hiding this comment.
Again, it causes an "unused import" warning which fails CI. In this case it is because health::protobuf::health already re-exports the contents of health::protobuf::health_grpc, so the line above already covers re-exporting it here.
| @@ -262,7 +262,7 @@ impl<'a> From<&'a GrpcSlice> for GrpcByteBuffer { | |||
| /// Create a buffer from the given single slice. | |||
| /// | |||
| /// A buffer, which length is 1, is allocated for the slice. | |||
| #[allow(clippy::cast_ref_to_mut)] | |||
| #[allow(invalid_reference_casting)] | |||
There was a problem hiding this comment.
Does it work for old rustc versions?
There was a problem hiding this comment.
It gives a warning in 1.71 and earlier, whereas clippy::cast_ref_to_mut gives a lint error in 1.72 and newer.
| @@ -592,6 +592,11 @@ struct ChannelInner { | |||
| channel: *mut grpc_channel, | |||
| } | |||
|
|
|||
| // SAFETY: `grpc_channel` is safe to send between threads, and `Environment` is already `Send`. | |||
| unsafe impl Send for ChannelInner {} | |||
There was a problem hiding this comment.
It's intended to impl for Channel instead. Because only the APIs exposed by Channel are Sync and Send.
There was a problem hiding this comment.
ChannelInner is not public so this doesn't affect the public API, and is cleaner than having a non-Sync type in an Arc.
There was a problem hiding this comment.
It's about maintenance. For example, putting ChannelInner in Arc can be unsafe until developer takes care of the wrappers. Marking it sync and send gives the wrong suggestion to developers who may simply put it into another wrapper without thinking twice.
| @@ -298,7 +299,7 @@ pub type BoxHandler = Box<dyn CloneableHandler>; | |||
| #[derive(Clone)] | |||
| pub struct RequestCallContext { | |||
| server: Arc<ServerCore>, | |||
| registry: Arc<UnsafeCell<HashMap<&'static [u8], BoxHandler>>>, | |||
| registry: Rc<UnsafeCell<HashMap<&'static [u8], BoxHandler>>>, | |||
There was a problem hiding this comment.
Because this struct can be send to different threads, using Rc is not safe.
There was a problem hiding this comment.
UnsafeCell<HashMap<&[u8], Box<dyn CloneableHandler>>> is not Sync, so Arc<UnsafeCell<HashMap<&'static [u8], BoxHandler>>> is neither Send nor Sync, and is not safe to send between threads. The only place that registry is used is by RequestCallContext::get_handler, which is unsafe and notes that it must always be called from the same thread.
If this safety requirement is not being met then the code is already unsound. If it is being met, then using an Rc rather than an Arc is fine.
There was a problem hiding this comment.
If you actually want to share it between threads, then this should use a Mutex rather than an UnsafeCell.
There was a problem hiding this comment.
Sent #645 to do this.
There was a problem hiding this comment.
is neither Send nor Sync, and is not safe to send between threads.
It's true generally speaking. But it's safe here because each request call will be bound to a thread and never be accessed from different threads at the same time. Using Arc instead of Rc because it can still be sent around, hence may be clone and drop from different threads at the same time. For example,
| thread 1 | thread 2 |
|---|---|
| clone | |
| send -> | |
| access handler | |
| drop | drop |
|
Covered by #648, thanks for your contribution! |
There are a number of CI failures in master, which makes it difficult to see when PRs are actually introducing new issues. This PR fixes most of them, apart from the Windows ones.