[SERVER-54356] make a ReturnableScopeGuard type Created: 05/Feb/21 Updated: 06/Dec/22 |
|
| Status: | Backlog |
| Project: | Core Server |
| Component/s: | Internal Code |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Improvement | Priority: | Major - P3 |
| Reporter: | Mathias Stearn | Assignee: | Backlog - Service Architecture |
| Resolution: | Unresolved | Votes: | 1 |
| Labels: | sa-remove-fv-backlog-22 | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Assigned Teams: |
Service Arch
|
| Participants: |
| Description |
|
It seems like it would be nice to be able to return a ScopeGuard-like type from a function. Eg, construct a guard at the top of a pushState() function to restore it to the then-current state prior to modifying, then return it so that the caller of pushState() will always automatically pop its state modification.
Based on a slack discussion, we decided it would be best to be a separate type to avoid breaking the rule that it is always safe (and preferred!) to use [&] captures with makeGuard (unless you are literally typing return makeGuard([&]{...}) but then you know what you are doing and deserve to win stupid prizes). From the same discussion, it would be safer if the type only supported move construction but not move assignment. This allows the important use case of returning a guard, but disallows silly operations like swapping two guards. |
| Comments |
| Comment by Andrew Shuvalov (Inactive) [ 18/Feb/21 ] |
|
Billy Donahue billy.donahue : |
| Comment by Andrew Shuvalov (Inactive) [ 18/Feb/21 ] |
|
"""Andrew Shuvalov Today at 2:40 PM Sam 1 hour ago Sam 1 hour ago Andrew Shuvalov 18 minutes ago Billy Donahue 15 minutes ago Billy Donahue 12 minutes ago Andrew Shuvalov 11 minutes ago Billy Donahue 10 minutes ago Sam 9 minutes ago Sam 9 minutes ago Andrew Shuvalov 6 minutes ago Billy Donahue 4 minutes ago Billy Donahue 4 minutes ago andy 3 minutes ago Andrew Shuvalov 2 minutes ago andy 2 minutes ago andy 1 minute ago andy < 1 minute ago |
| Comment by Andy Schwerin [ 15/Feb/21 ] |
|
I could accept a const based solution instead of a separate type. But how many places are we actually returning guard objects that we could turn into makeGuard? Is this functionality we want? None of the ones in Interruptible we’re good examples, hence my plan to remove them. |
| Comment by Billy Donahue [ 15/Feb/21 ] |
|
This line of reasoning reminds me of the case for using more const local variables. Of the 194 makeGuard calls, 25 are initializing a const variable. If we see a const auto xxx = makeGuard(...) we know it's one of the easy cases. If it's guarding a block that's only <5 lines, it's also an easy case. > "Since 90% or more of our guards are just trying to always run code at block exit," It's actually not that high. It's only 73% (324/443). Of the entire population of 443 guards, 274 are declared undismissable (25 "const makeGuard" and 249 ON_BLOCK_EXIT) 119 are dismissed. 50 can be dismissed ("nonconst makeGuard") but aren't. The problem is then narrowed down to the 50 of 443 that are eligible for tightening up. All the others are already declared properly. We could add const to them or leave them alone. In a lot of these cases they're guarding a scope that's extremely small, like <5 lines. Sometimes just 1 line. There's no way to make those more clear, really. |
| Comment by Mathias Stearn [ 15/Feb/21 ] |
|
schwerin was more opposed to the unified type so I'll let him defend the separate types. Personally I'd be fine with a unified type, but if we go that route, I'd rather we replace all "simple" uses of makeGuard with ON_BLOCK_EXIT. While I already dislike code using makeGuard when the declarative ON_BLOCK_EXIT would work, currently it is still restricted to the current block and may or may not be dismissed. If we unify, then that is one more thing that I need to check for when I see a call to makeGuard in order to understand the code. Since 90% or more of our guards are just trying to always run code at block exit, we should save the explicit makeGuard for the interesting cases where more caution is needed by readers. |
| Comment by Billy Donahue [ 13/Feb/21 ] |
|
I strongly agree with the basic idea that a movable ScopeGuard is really useful. Instead of a new type, I propose the simpler change to just make ScopeGuard movable, and rename it Cleanup. The word ScopeGuard appears only 2 times in our codebase, vs 443 for ON_BLOCK_EXIT or makeGuard. So we can just rename it whatever we want. I like Cleanup, which is the name I gave the same class when I wrote it at my previous job. It's been changed a bit but it was released in an absl drop last month. I don't like the name ScopeGuard if it becomes movable. Its behavior isn't really bound to a scope anymore. ReturnableScopeGuard is almost a contradiction in terms. It still refers to "scope", and when its salient feature is its ability to escape from a scope this feels unclear. But that's just bikeshedding. On to the point about referential safety though, and the motivation for splitting off the new type, I don't feel there's a need to do that. We all know the well-known rule of thumb is that if the lambda doesn't leave the scope, you can always use [&]. I think people are pretty good at applying that rule, and they use [&] lambdas with generic algorithms like sort or remove_if, but not with, say, std::async, and when passing to functions with complex behaviors they know to choose bindings carefully. I don't think making a Cleanup object is a complex behavior, though. When passing any reference handle to any function you have to be on your toes and know whether the function being called is going to retain a copy or not. I think that declaring a local Cleanup is a lot like declaring a local std::function. There has to be an awareness that this new object now holds the lambda and its references. If it leaves the scope you have to think harder about the lambda reference captures. Similar to StringData too. If you let a StringData leave the scope you have to think about the lifetime of its referent character array. I meann to say that a movable, returnable Cleanup is familiar territory and we have the intuition required to deal with it. As for move-assignment, I think we don't really have a strong reason to forbid that either. If F can move-assign, just let it. Lambdas have deleted assignment operators so we're not talking about lambdas. If F is a nonlambda callable like std::function we don't have to fret about allowing conventional value-type operations, and should just let the users decide what's useful to them. Move-assignment is necessary to be able to store these things in containers or use them as data members of assignable value types. They're not fundamentally different from std::function in terms of lifetime concerns. They just have a special behavior that they're invoked upon destruction, but that's not so magical. Swapping Cleanup objects isn't silly. I guess if we call them ScopeGuard it sounds silly
|
| Comment by Mathias Stearn [ 05/Feb/21 ] |
|
This is better than writing custom types everwhere something like this is needed, both to avoid having to maintain the "am I dismissed" logic, and because [this, oldSize=foo.size()] { foo.resize(oldSize);} is a lot less boiler plate than writing a class with members, constructors and a destructor. |