[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 :
"So maybe there's a flexible Cleanup type and then the ScopeGuard stays on as derived from Cleanup, but restricting the functionality. Then it's just like 5 lines or something. I think that could work, yeah."

Comment by Andrew Shuvalov (Inactive) [ 18/Feb/21 ]

"""Andrew Shuvalov Today at 2:40 PM
Do we have a RAII ‘outstanding counter’, which adds/deletes something to something and does the opposite in destructor? If not I’ll write one.
17 replies

Sam 1 hour ago
You can use ScopeGuard to implement this — increment before making the guard and have the guard do the decrement via its callback.
:+1:
1

Sam 1 hour ago
Would that work for your use-case?

Andrew Shuvalov 18 minutes ago
Thanks, almost. I need to make it to track the long Future lifetime, to delete when the Future is at last step. Is it better to use unique_ptr wrapper around ScopeGuard and make the last callback to own it, or I need a shared_ptr and make more than 1 callback to own it?

Billy Donahue 15 minutes ago
You have hit an interesting issue with ScopeGuard that it's currently associated with scopes. I've suggested upgrading it to Cleanup and making it movable, for exactly the motivation you mention. To have a generalized cleanup action as a subobject that executes when its enclosing object (maybe a lambda) is destroyed. I think some of the hackery with unique_ptr deleters could be alleviated by such a thing.
https://jira.mongodb.org/browse/SERVER-54356

Billy Donahue 12 minutes ago
If it's common that people consider making scoped_ptr and unique_ptr and optional wrappers just to get the lifetime semantics they want out of something then it feels like our offering might be not quite right.

Andrew Shuvalov 11 minutes ago
Ah, totally onboard, and it should be a separate type. Move constructor without move assignment should enforce proper use. I will use uniquer_ptr for now.

Billy Donahue 10 minutes ago
DO I Disagree about separate type? Maybe I don't know what you're suggesting. (edited)

Sam 9 minutes ago
My understanding is that you want the callback to run as the future chain is being destroyed. If that’s the case, I believe unique_ptr is the right choice until we can have the lambda for the last continuation to capture an instance of ScopeGuard.

Sam 9 minutes ago
Is this what you’re trying to achieve?

Andrew Shuvalov 6 minutes ago
Yes. The only reason for a separate type is that the ScopeGuard is 20 lines. Making another 20 lines type with very clear separation is better than extending ScopeGuard to 30 lines and adding 10 lines of comments explaining different usage patterns.

Billy Donahue 4 minutes ago
Oh yeah then I do disagree with you. I don't think the implementation should factor into the decision. The docs need to be good enough as always. I think they can be structured so that users don't get bogged down in learning features they won't use.

Billy Donahue 4 minutes ago
I would prefer to have fewer types in the zoo, even if it means one of them has extra functionality.

andy 3 minutes ago
I’m in the 2-types camp, I think. It’s helpful to a reviewer to look at some code and answer the question “is it OK that the lambda passed to this guard captures [&]?” Using “const” in the capture for that as the answer, I can’t decide if it’s too subtle or not.

Andrew Shuvalov 2 minutes ago
Ok, hopefully we are smart enough to RTM :slightly_smiling_face:

andy 2 minutes ago
If we’re truly smart, there won’t need to be a manual.

andy 1 minute ago
I would ideally like to have our support libraries usable by people who want to use them, not people who want to know everything about them.

andy < 1 minute ago
I wouldn’t be surprised (or disappointed) to lose this debate, though. (edited)
"""

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.
More const could help here too. A const ScopeGuard can't move, can't be returned, and can't be dismissed. We could add const to guard declarations to mark them undismissable and immovable.

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.
https://github.com/abseil/abseil-cpp/blob/master/absl/cleanup/cleanup.h

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  but it's logically fine. Those objects are generalized destructor behavior and in that case they could be used as data members of other objects, serving pretty much the same boilerplate-saving role as they did in the example given above with the foo.resize(oldSize) code. E.g. you can bind a Cleanup expression to a lambda and then move the lambda so that it carries its cleanup action along whereever it goes.

 

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.

Generated at Thu Feb 08 05:33:18 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.