-
Notifications
You must be signed in to change notification settings - Fork 1.9k
RAII chapter for idiomatic rust #2820
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: main
Are you sure you want to change the base?
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
src/idiomatic/leveraging-the-type-system/raii/drop_limitations.md
Outdated
Show resolved
Hide resolved
src/idiomatic/leveraging-the-type-system/raii/drop_limitations.md
Outdated
Show resolved
Hide resolved
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.
Looks pretty good, there's lots of good stuff in this chapter 😁 I've got various feedback below, but it's definitely off to a good start!
I started to action all feedback since today. I'll explicitly re-request reviews once I am done. Probably best not to review again until then. |
- directly where possible - otherwise as inline feedback as notes to take into account for next draft
refresher on `RAII` and use the OS File Descriptor example to start the discussions around RAII all previous content is for now moved to `_old` for my own reference, will add the new content based on the agreed upon new structure next.
The four slides in the RAII chapter are ready for review again. |
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 looking pretty good. The drop bomb and scope guard slides look good to me, my biggest suggestion is that we might want to split the mutex slide up so that it flows a bit better.
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 had a brief look at the updates since my last comments, and I'm happy enough!
split mutex (RAII) chapter in two parts
@randomPoison your feedback was all address as far as I know. Ready for another round of reviews. |
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.
Thanks for addressing my previous feedback! I've looked over things again and still have some suggestions/requests for how this can be improved. There's a couple of places where I've suggested adding new slides:
- Splitting out the discussion of when
drop
does and doesn't run. - Using
mem::forget
to defuse drop bombs. - Using
Option
for cases where you need to transfer ownership indrop
.
I'm fine with those being split into a separate PR if you want to get this one landed, though if you do that then it'd be good to open an issue tracking the unfinished changes to this section.
- If both `drop()` and `close()` exist, the file descriptor may be released | ||
twice. To avoid this, remove `close()` and rely solely on `Drop`. |
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.
Having both close
and drop
is fine, but the close
impl needs to mem::forget
(or destructure) the File
object to prevent drop
from also running. That's a normal pattern, especially when close
potentially wants to report an error, which can't be done with drop
.
I think that's a point worth covering in its own slide. We point out that drop
can't return any errors, I think it'd be good to talk about that directly in a slide and demonstrate how you can create a function like close
that consumes the object and prevent the normal destructor from running redundantly.
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.
+1, agreed. There are cases where it is acceptable define both drop
(which does best-case cleanup and ignores or merely logs errors) and something like close
(which returns an error). I think it would be worthwhile to demonstrate this.
- When is `Drop::drop` called? | ||
|
||
Normally, when the `file` variable in `main` goes out of scope (either on | ||
return or due to a panic), `drop()` is called automatically. | ||
|
||
If the file is moved into another function, for example `read_all()`, the | ||
value is dropped when that function returns — not in `main`. | ||
|
||
In contrast, C++ runs destructors in the original scope even for moved-from | ||
values. | ||
|
||
- The same mechanism powers `std::mem::drop`: | ||
|
||
```rust | ||
pub fn drop<T>(_x: T) {} | ||
``` | ||
|
||
You can use it to force early destruction of a value before its natural end of | ||
scope. | ||
|
||
- Insert `panic!("oops")` at the start of `read_to_end()` to show that `drop()` | ||
still runs during unwinding. | ||
|
||
- There are cases where destructors will not run: | ||
- If a destructor itself panics during unwinding, the program aborts | ||
immediately. | ||
- If the program exits with `std::process::exit()` or is compiled with the | ||
`abort` panic strategy, destructors are skipped. |
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 still think this is worth pulling out into its own slide (I previously commented that, idk if it got lost in the shuffle).
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.
+1, agreed.
#[derive(Debug)] | ||
struct Mutex<T> { | ||
value: std::cell::UnsafeCell<T>, | ||
is_locked: std::sync::atomic::AtomicBool, | ||
} | ||
|
||
#[derive(Debug)] | ||
struct MutexGuard<'a, T> { | ||
value: &'a mut T, | ||
mutex: &'a Mutex<T>, | ||
} | ||
|
||
impl<T> Mutex<T> { | ||
fn new(value: T) -> Self { | ||
Self { | ||
value: std::cell::UnsafeCell::new(value), | ||
is_locked: std::sync::atomic::AtomicBool::new(false), | ||
} | ||
} | ||
|
||
fn lock(&self) -> MutexGuard<'_, T> { | ||
// Acquire the lock and create the guard object. | ||
if self.is_locked.swap(true, std::sync::atomic::Ordering::AcqRel) { | ||
todo!("Block until the lock is released"); | ||
} | ||
let value = unsafe { &mut *self.value.get() }; | ||
MutexGuard { value, mutex: self } | ||
} | ||
} | ||
|
||
impl<'a, T> Drop for MutexGuard<'a, T> { | ||
fn drop(&mut self) { | ||
self.mutex.is_locked.store(false, std::sync::atomic::Ordering::Release); | ||
} | ||
} | ||
|
||
fn main() { | ||
let m = Mutex::new(vec![1, 2, 3]); | ||
|
||
let mut guard = m.lock(); | ||
guard.value.push(4); | ||
guard.value.push(5); | ||
println!("{guard:?}"); | ||
} |
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 code example is very verbose, I think it'd be better to simplify this. I think we should remove the atomics and UnsafeCell
, and simplify this to a dummy mutex that just locks and unlocks (i.e. doesn't actually wrap any data). I think we can also remove the main
function since we already showed what using the mutex looks like on the previous slide. My suggestion would be something like this:
#[derive(Debug)]
struct Mutex {
is_locked: bool,
}
#[derive(Debug)]
struct MutexGuard<'a> {
mutex: &'a mut Mutex<T>,
}
impl Mutex {
fn new() -> Self {
Self {
is_locked: false,
}
}
fn lock(&mut self) -> MutexGuard<'_> {
self.is_locked = true;
MutexGuard { mutex: self }
}
}
impl<'a> Drop for MutexGuard<'a> {
fn drop(&mut self) {
self.mutex.is_locked = false;
}
}
This is obviously not a realistic mutex, but it's far clearer from the perspective of illustrating the drop guard pattern to students. I think it'd be enough to have a speaker note that points out that this is not accurate to how Mutex
is actually implemented.
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 definitely agree that the example is too long for a single slide and for the point we're trying to illustrate (drop guards). I like your suggested simplified code. I would like to suggest a few things in addition:
-
add a "// TODO: make atomic for production" comment after "is_locked: bool,"
-
at first removing the value being protected felt like a significant departure from the original mutex. Then I realized that it only felt significant because I was so focused on Rust, but it is actually a pretty common mutex. So, my suggestion is: we should ground this example in how mutexes are implemented in other programming languages, where they are essentially atomic booleans and some kind of waiting primitive. This should be added to the instructor notes. It is however important to explain so that the students understand what we're going for with this example. tl;dr: I think it is better to say that we're trying to implement an equivalent of a mutex say from C++, rather than a simplified Rust mutex.
Something like this:
-
This example shows a C++-style mutex that does not contain the data being protected. This would be deeply non-idiomatic in Rust, but this example only illustrates the core idea of a drop guard. It does not attempt to show a good way of implementing a mutex in Rust.
-
For brevity, we've omitted several features:
- value being protected,
- ergonomic access via the Deref and DerefMut traits,
- a truly blocking .lock() method,
- correct handling of panics.
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 think it's fine to have a note that this is not a realistic example, but I'd suggest keeping it concise to avoid distracting from the actual point of the slide (i.e demonstrating the guard pattern). I'd prefer not to have a TODO comment in the example code, that just adds noise and isn't really relevant since this is not trying to be an accurate mutex implementation. I also don't think it's necessary to say that this is like a C++ mutex, the distinction between a mutex that wraps data vs a mutex that protects a critical section is entirely tangential to the guard object pattern.
If you want to describe how this example differs from a real mutex, or make a comparison to C++ mutexes, then I'd prefer that those go into a "More to Explore" section at the end of the notes to avoid cluttering the speaker notes section.
In earlier examples, RAII was used to manage concrete resources like file | ||
descriptors. With a `Mutex`, the resource is more abstract: exclusive access to | ||
a value. | ||
|
||
Rust models this using a `MutexGuard`, which ties access to a critical section | ||
to the lifetime of a value on the stack. |
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.
In earlier examples, RAII was used to manage concrete resources like file | |
descriptors. With a `Mutex`, the resource is more abstract: exclusive access to | |
a value. | |
Rust models this using a `MutexGuard`, which ties access to a critical section | |
to the lifetime of a value on the stack. | |
In earlier examples, RAII was used to manage concrete resources like file | |
descriptors. With a `Mutex`, the "resource" is mutable access to a value. | |
You access the value by calling `lock`, which then returns a `MutexGuard` | |
which will unlock the `Mutex` automatically when dropped. |
A couple suggestions for wording here:
- I wouldn't say the resource is "abstract", it's just not an external resource the way that a heap allocation or a file handle is.
- For this slide I think it'd be better to explain the usage of
Mutex
so that on the next slide we can dig into howMutexGuard
works.
A **drop guard** in Rust is a temporary _RAII_ guard that executes a specific | ||
action when it goes out of scope. | ||
|
||
It acts as a wrapper around a value, ensuring that some cleanup or secondary | ||
behavior happens automatically when the guard is dropped. | ||
|
||
One of the most common examples is `MutexGuard`, which represents temporary | ||
exclusive access to a shared resource. |
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.
A **drop guard** in Rust is a temporary _RAII_ guard that executes a specific | |
action when it goes out of scope. | |
It acts as a wrapper around a value, ensuring that some cleanup or secondary | |
behavior happens automatically when the guard is dropped. | |
One of the most common examples is `MutexGuard`, which represents temporary | |
exclusive access to a shared resource. | |
A **drop guard** in Rust is a temporary object that performs | |
some kind of cleanup when it goes out of scope. In the case | |
of `Mutex`, the `lock` method returns a `MutexGuard` that | |
automatically unlocks the mutex on drop: |
I think more concise wording here would be good to save space for the larger example code on this slide.
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.
+1, agreed. You can move the longer version to a bullet in the speaker notes.
let tx = Transaction::start(); | ||
|
||
if some_condition() { | ||
tx.commit()?; | ||
} else { | ||
tx.rollback()?; | ||
} | ||
|
||
// Uncomment to see the panic: | ||
// let tx2 = Transaction::start(); |
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.
let tx = Transaction::start(); | |
if some_condition() { | |
tx.commit()?; | |
} else { | |
tx.rollback()?; | |
} | |
// Uncomment to see the panic: | |
// let tx2 = Transaction::start(); | |
let tx = Transaction::start(); | |
// Use `tx` to build the transaction, then commit it. | |
// Comment out the call to `commit` to see the panic. | |
tx.commit()?; |
Let's simplify this by removing rollback
and tx2
. That will help the example fit on the slide better.
|
||
fn some_condition() -> bool { | ||
// [...] | ||
true | ||
} |
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 can also be removed once rollback
is removed.
- [`Option<T>` with `.take()`](https://doc.rust-lang.org/std/option/enum.Option.html#method.take): | ||
A common pattern inside `Drop` to move out internal values and prevent double | ||
drops. | ||
|
||
```rust,compile_fail | ||
impl Drop for MyResource { | ||
fn drop(&mut self) { | ||
if let Some(handle) = self.handle.take() { | ||
// do cleanup with handle | ||
} | ||
} | ||
} | ||
``` |
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 think this deserves its own slide. Having to move out of a field in drop
comes up pretty frequently, and this is a common enough pattern that I think it's worth demonstrating explicitly. That can be split out into its own PR if you want.
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.
+1, agreed.
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.
The example on this slide demonstrates using a runtime flag to track if the transaction has been finalized, and then checks that flag to conditionally panic in drop
. This is a reasonable approach to take, but it has the drawback of some additional runtime overhead. There's another way to achieve the same thing without any overhead: use mem::forget
in commit
to directly prevent drop
from running.
I'd like to discuss forget
somewhere in this section, since it's very relevant when using Drop
, especially in cases where you sometimes want to prevent the regular destructor from running. In another comment I suggested splitting out a slide that discusses when drop
is and isn't run, discussing forget
there might make sense. Otherwise, it wouldn't hurt to have another slide that demonstrates this transaction example using forget
instead of the active
flag.
- If both `drop()` and `close()` exist, the file descriptor may be released | ||
twice. To avoid this, remove `close()` and rely solely on `Drop`. |
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.
+1, agreed. There are cases where it is acceptable define both drop
(which does best-case cleanup and ignores or merely logs errors) and something like close
(which returns an error). I think it would be worthwhile to demonstrate this.
|
||
```rust | ||
pub fn drop<T>(_x: T) {} | ||
``` |
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.
The instructor can't speak such a long code example. This needs to be put on a separate slide.
- When is `Drop::drop` called? | ||
|
||
Normally, when the `file` variable in `main` goes out of scope (either on | ||
return or due to a panic), `drop()` is called automatically. | ||
|
||
If the file is moved into another function, for example `read_all()`, the | ||
value is dropped when that function returns — not in `main`. | ||
|
||
In contrast, C++ runs destructors in the original scope even for moved-from | ||
values. | ||
|
||
- The same mechanism powers `std::mem::drop`: | ||
|
||
```rust | ||
pub fn drop<T>(_x: T) {} | ||
``` | ||
|
||
You can use it to force early destruction of a value before its natural end of | ||
scope. | ||
|
||
- Insert `panic!("oops")` at the start of `read_to_end()` to show that `drop()` | ||
still runs during unwinding. | ||
|
||
- There are cases where destructors will not run: | ||
- If a destructor itself panics during unwinding, the program aborts | ||
immediately. | ||
- If the program exits with `std::process::exit()` or is compiled with the | ||
`abort` panic strategy, destructors are skipped. |
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.
+1, agreed.
@GlenDC Thank you for the updates! Please see my latest round of comments. There are a lot of comments, but I tried to make them actionable. Please pay particular attention to the requests to split the slides. Looking forward to seeing the revised section, I think this PR is getting closer and closer to being finalized. |
This PR adds the RAII chapter for the idiomatic Rust deep dive.