fix(parity-clib): grumbles that were not addressed in #9920 #10154
Conversation
niklasad1
added
A0-pleasereview 🤓
niklasad1
changed the title
niklasad1
force-pushed
the
parity-clib/grumbles
branch
from
fdee6ff
to
c14ce30
Compare
sorpaas
added
A8-looksgood 🦄
|
Needs a 2nd review |
| unsafe impl<'a> Sync for Callback<'a> {} | ||
| impl<'a> Callback<'a> { | ||
| unsafe impl<'a> Send for JavaCallback<'a> {} | ||
| unsafe impl<'a> Sync for JavaCallback<'a> {} |
There was a problem hiding this comment.
It wasn't introduced by this PR, but could you explain why it's safe for GlobalRef?
There was a problem hiding this comment.
I don't understand what you mean. Can you elaborate a bit?
There was a problem hiding this comment.
Sorry, I meant why is it safe to impl Sync and Send for JavaCallback?
There was a problem hiding this comment.
That is a good question I have to admit I'm not completely sure (I more or less copy-pasted it from the lib.rs) but I argue that JavaVM and GlobalRef is never mutated and the Java code is thread-safe!
Also both JavaVM GlobalRef have unsafe Send + Sync impls but an inner-type GlobalRefGuard doesn't with the unsafe impls I get the following error:
53 | impl<'a> Callback for JavaCallback<'a> {
| ^^^^^^^^ `*mut jni::sys::_jobject` cannot be shared between threads safely
|
= help: within `jni::objects::global_ref::GlobalRefGuard`, the trait `std::marker::Sync` is not implemented for `*mut jni::sys::_jobject`
= note: required because it appears within the type `jni::objects::JObject<'static>`
= note: required because it appears within the type `jni::objects::global_ref::GlobalRefGuard`
= note: required because of the requirements on the impl of `std::marker::Sync` for `std::sync::Arc<jni::objects::global_ref::GlobalRefGuard>`
= note: required because it appears within the type `jni::objects::GlobalRef`
= note: required because it appears within the type `java::JavaCallback<'a>`Thoughts?
There was a problem hiding this comment.
The reason I asked, is because the docs for GlobalRef 0.10.2 showed
impl !Sync for GlobalRef It seems like Sync was added later (jni-rs/jni-rs#145), and the implementation hasn't changed for GlobalRef.
There was a problem hiding this comment.
Thanks good point let’s ask them to bump the version so we can get rid of the unsafe part then :)
These functions can still end up with `UB` thus should be unsafe
niklasad1
force-pushed
the
parity-clib/grumbles
branch
from
c14ce30
to
27b9d49
Compare
| unsafe impl<'a> Sync for Callback<'a> {} | ||
| impl<'a> Callback<'a> { | ||
| unsafe impl<'a> Send for JavaCallback<'a> {} | ||
| unsafe impl<'a> Sync for JavaCallback<'a> {} |
There was a problem hiding this comment.
The reason I asked, is because the docs for GlobalRef 0.10.2 showed
impl !Sync for GlobalRef It seems like Sync was added later (jni-rs/jni-rs#145), and the implementation hasn't changed for GlobalRef.
* master: no volumes are needed, just run -v volume:/path/in/the/container (#10345) Fixed misstype (#10351) snap: prefix version and populate candidate channel (#10343) Bundle protocol and packet_id together in chain sync (#10315) role back docker build image and docker deploy image to ubuntu:xenial based (#10338) change docker image based on debian instead of ubuntu due to the chan… (#10336) Don't add discovery initiators to the node table (#10305) fix(docker): fix not receives SIGINT (#10059) snap: official image / test (#10168) fix(add helper for timestamp overflows) (#10330) Additional error for invalid gas (#10327) Revive parity_setMinGasPrice RPC call (#10294) Add Statetest support for Constantinople Fix (#10323) fix(parity-clib): grumbles that were not addressed in #9920 (#10154) fix(light-rpc): Make `light_sync` generic (#10238) fix publish job (#10317) Secure WS-RPC: grant access to all apis (#10246) Make specification of protocol in SyncRequester::send_request explicit (#10295)
This PR introduces the following:
libandjava(introduces a trait Callback to pass instead of the concrete type)