-
-
Notifications
You must be signed in to change notification settings - Fork 369
Remove potentially dangerous Send bound #1571
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
base: dev
Are you sure you want to change the base?
Conversation
StaticValue<T> is simply a wrapper around Mutex<T>. For Mutex<T> to be Send or Sync the stdlib requires T: Send. This bound is missing on the Send and Sync impl of StaticValue. The Send impl is not even needed, remove it.
Send
boundSend
bound
Send
bound
what's the advantage of removing the Send bound if Sync is still there? |
Is removing unsafe code not a general positive? |
Package Changes Through 5549ba3No changes. Add a change file through the GitHub UI by following this link. Read about change files or the docs at github.com/jbolda/covector |
well in this case i think removing unsafe code would mean making the StaticValue actually safe to share between threads (which is a breaking change like you said - one that Tauri cannot adhere without a breaking change on its own). |
This PR does what it says on the tin, it removes unsafe code. Whether the behavior changes is a bit tangential, every bit of If Can we hold off on merging? The missing bound on the closure seems unsound, but I can't remember what I got stuck on last time I tried to confirm this. |
This is good to merge. It would probably make sense to update the bounds and do the breaking change by bounding all closures with |
StaticValue
hasSend
andSync
impls with unclear soundness.StaticValue<T>
is a wrapper aroundMutex<T>
, butMutex<T>
itself is only everSend
orSync
whenT: Send
. I tried to get something to crash but the'static
lifetime on the closures seems quite restrictive.To remove any doubts about soundness the involved closures would need an additional
Send
bound, a breaking change.This PR removes the
Send
impl onStaticValue
since it is never needed. The questionableSync
impl remains.