-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
refactor(permissions): add PermissionsContainer struct for internal mutability #17134
Conversation
let permissions = if is_dynamic { | ||
self.dynamic_permissions.clone() | ||
} else { | ||
self.root_permissions.clone() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fixing actual fix of the issue linked in this PR - because we were passing a mutable reference to a future the updated state of permissions was never preserved - because we were cloning the struct here and never updating it after future resolves.
6363ee9
to
5d5892c
Compare
5d5892c
to
eee7b55
Compare
runtime/permissions/mod.rs
Outdated
@@ -34,6 +36,8 @@ pub use prompter::PromptCallback; | |||
static DEBUG_LOG_ENABLED: Lazy<bool> = | |||
Lazy::new(|| log::log_enabled!(log::Level::Debug)); | |||
|
|||
pub type PermissionsContainer = Arc<Mutex<Permissions>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If every usage of Permissions is replaced with PermissionsContainer, then why not just include that in the actual struct. IE
struct Permissions(Arc<Mutex<Inner>>)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not every usage is replaced with PermissionsContainer
- usages in ext/
crates are using Arc<Mutex<P>>
which depend on traits from various crates. I tried the approach you suggest but it doesn't worked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
usages in ext/ crates are using Arc<Mutex
> which depend on traits from various crates
what's an example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Examples:
ext/net/ops.rs
ext/net/ops_tls.rs
ext/fetch/lib.rs
These ops don't have access to Permissions
struct (because runtime/
crate depends on ext/net
and ext/fetch
) and thus to PermissionsContainer
. Extension crates define traits for permission checks that have matching signatures that are later implemented in runtime/
for Permissions
struct (and thus PermissionsContainer
since it derefs to Permissions
). This is somewhat optimal solution because it doesn't introduce tight coupling between ext/
crates and runtime/
thanks to Rust's trait system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get it. Let's take op_net_send_udp
for example. Here it gets a NetPermissions object from the state:
Lines 166 to 169 in 319f607
s.borrow_mut::<NP>().check_net( | |
&(&addr.hostname, Some(addr.port)), | |
"Deno.DatagramConn.send()", | |
)?; |
How is changing Permissions to be struct Permissions(Arc<Mutex<Inner>>)
going to affect that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, let’s make this a struct that buries this type (could make PermissionsContainer a struct). Lots of noise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rewrote it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's still a PermissionsContainer tho?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To reduce scenarios where we acquire a lock multiple times, I believe we still need one type for the unlocked state then one for the locked state.
One thing we could maybe do is hide the mutex within the unlocked state type's interface, but I'm not sure it's worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's still a PermissionsContainer tho?
Yes, there are still situations where you need to get to the inner structure, eg. in CLI - I'd much rather handle that via Permissions
struct than some Inner
.
runtime/permissions/mod.rs
Outdated
@@ -34,6 +36,8 @@ pub use prompter::PromptCallback; | |||
static DEBUG_LOG_ENABLED: Lazy<bool> = | |||
Lazy::new(|| log::log_enabled!(log::Level::Debug)); | |||
|
|||
pub type PermissionsContainer = Arc<Mutex<Permissions>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, let’s make this a struct that buries this type (could make PermissionsContainer a struct). Lots of noise.
c541b23
to
e0a2b09
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -87,7 +85,7 @@ fn op_set_env( | |||
key: String, | |||
value: String, | |||
) -> Result<(), AnyError> { | |||
state.borrow_mut::<Permissions>().env.check(&key)?; | |||
state.borrow_mut::<PermissionsContainer>().check_env(&key)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess these don't really need to be borrow_mut anymore. I don't think it matters though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah true. I'll change that to regular borrows
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I will leave it as is - traits from various ext/
crates require mutable reference (because they don't know that we're storing this behind Arc<Mutex<>>
and it feels better to have consistent signature (also suggests that it can be internally mutated).
…utability (#17134) Turns out we were cloning permissions which after prompting were discarded, so the state of permissions was never preserved. To handle that we need to store all permissions behind "Arc<Mutex<>>" (because there are situations where we need to send them to other thread). Testing and benching code still uses "Permissions" in most places - it's undesirable to share the same permission set between various test/bench files - otherwise granting or revoking permissions in one file would influence behavior of other test files.
…utability (#17134) Turns out we were cloning permissions which after prompting were discarded, so the state of permissions was never preserved. To handle that we need to store all permissions behind "Arc<Mutex<>>" (because there are situations where we need to send them to other thread). Testing and benching code still uses "Permissions" in most places - it's undesirable to share the same permission set between various test/bench files - otherwise granting or revoking permissions in one file would influence behavior of other test files.
Fixes #16772.
Turns out we were cloning permissions which after prompting were discarded,
so the state of permissions was never preserved. To handle that we need to store
all permissions behind "Arc<Mutex<>>" (because there are situations where we
need to send them to other thread).
Testing and benching code still uses "Permissions" in most places - it's undesirable
to share the same permission set between various test/bench files - otherwise
granting or revoking permissions in one file would influence behavior of other test
files.