 |
C++Talk.NET
C++ language newsgroups
|
| View previous topic :: View next topic |
| Author |
Message |
Louis-Philippe Gagnon
Guest
|
 Posted: Fri Dec 17, 2004 11:05 am Post subject: ScopeGuard template: how to use safely? |
 |
|
Nearly 2 years ago, I came across the ScopeGuard CUJ article by Andrei
Alexandrescu and Petru Marginean [1]. It was my first exposure to the
"true power" of templates, and I haven't looked back since.
That is, until last night, when I realized I was about to use a
scopeguard-like class[2] in a decidedly exception-unsafe way. Thinking
something might have been lost in translation, I went back to the CUJ
article to see if the issue had been addressed. It didn't seem to be.
So now, filled with newfound doubt, I come to the experts seeking
comfort.
First, the final example from the CUJ article:
==
{
FILE* topSecret = fopen("cia.txt");
ON_BLOCK_EXIT(std::fclose, topSecret);
... use topSecret ...
} // topSecret automagically closed
==
All perfectly safe, no problem there. (well, aside from a missing
parameter to fopen ;)
But let's tweak it a bit:
==
/* self-explanatory wrappers */
FILE logging_fopen(std::string const&,std::string const&);
void logging_fclose(FILE*, std::string const& name);
void broken(std::string const& name)
{
FILE* lessSecret = logging_fopen(name,"r");
ON_BLOCK_EXIT(logging_fclose, lessSecret, name);
... use lessSecret ...
} // lessSecret automagically closed..?
==
Digging in, we go through ON_BLOCK_EXIT, ScopeGuard, MakeGuard, and
end up in ScopeGuardImpl1, where we find:
ScopeGuardImpl1(const Fun& fun, const Parm& parm)
: fun_(fun), parm_(parm)
[...]
Fun fun_;
const Parm parm_;
In my example, we'd have a ScopeGuardImpl2, and there would be a
"const Parm2 parm2" member.
If I followed the path of template instantiations correctly, "Parm2"
in this case would be std::string.
A std::string held by value.
Meaning MakeGuard can throw, in which case the file won't get closed.
Did I make a wrong turn somewhere?
Now, there are workarounds, of course; for example, it looks like the
RefHolder class presented at the end of the article could be used to
avoid the unwanted copy. But this seems to greatly reduce ScopeGuard's
ease of use.
So the questions are:
- Is this a known issue of the idiom?
- If so, are there 'usual' ways to avoid it?
- Would ScopeGuard-like classes be better designed if they kept
references to their parameters rather than copies, or would lifetime
issues make the cure worse than the disease?
- Was I the only one lulled into a false sense of security?
If this has been discussed before, please point me at the relevant
material. Google doesn't seem to want to help.
[1] http://www.cuj.com/documents/s=8000/cujcexp1812alexandr/
[2] Truth time: my "scopeguard-like" class actually uses a
boost::function-like back-end. Since dynamic allocation is (afaik)
unavoidable with this approach, *all* uses of such a scopeguard class
would be exception-unsafe.
--
LPG
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
 |
Branimir Maksimovic
Guest
|
| Posted: Fri Dec 17, 2004 1:49 pm Post subject: Re: ScopeGuard template: how to use safely? |
|
|
Louis-Philippe Gagnon wrote:
| Quote: | Nearly 2 years ago, I came across the ScopeGuard CUJ article by
Andrei
Alexandrescu and Petru Marginean [1]. It was my first exposure to the
"true power" of templates, and I haven't looked back since.
That is, until last night, when I realized I was about to use a
scopeguard-like class[2] in a decidedly exception-unsafe way.
........
ScopeGuardImpl1(const Fun& fun, const Parm& parm)
: fun_(fun), parm_(parm)
[...]
Fun fun_;
const Parm parm_;
|
Just put
const Parm& parm_;
// works the same and you don't have copy construction
// that can throw
Greetings, Bane.
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
|
Paavo Helde
Guest
|
| Posted: Sat Dec 18, 2004 12:23 pm Post subject: Re: ScopeGuard template: how to use safely? |
|
|
Louis-Philippe Gagnon <lp_gagnon (AT) videotron (DOT) ca> wrote in
news:50c4s0ln1tmri5l4t56hjoq7s0nif48d0t (AT) 4ax (DOT) com:
| Quote: |
void broken(std::string const& name)
{
FILE* lessSecret = logging_fopen(name,"r");
ON_BLOCK_EXIT(logging_fclose, lessSecret, name);
... use lessSecret ...
} // lessSecret automagically closed..?
[...]
A std::string held by value.
Meaning MakeGuard can throw, in which case the file won't get closed.
|
To my mind, ON_BLOCK_EXIT is kind of destructor, meaning that it has to
be no-throw. If you pass any kind of arguments with throwing copy ctor,
then you violate this silent assumption.
The remedy is simple: if the copy ctor can throw, then avoid it:
FILE* lessSecret = logging_fopen(name,"r");
ON_BLOCK_EXIT(logging_fclose, lessSecret, ByRef(name));
If I understand this correctly, this packs only a reference to 'name' in
the ScopeGuard object, thus avoiding the copy.
Finally, I would not worry too much about filename copy ctor throwing.
IMHO a stack overflow can happen much more likely, leading directly to
undefined behaviour.
HTH
Paavo
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
|
Peter Dimov
Guest
|
| Posted: Sat Dec 18, 2004 12:32 pm Post subject: Re: ScopeGuard template: how to use safely? |
|
|
Louis-Philippe Gagnon wrote:
[...]
| Quote: | /* self-explanatory wrappers */
FILE logging_fopen(std::string const&,std::string const&);
void logging_fclose(FILE*, std::string const& name);
void broken(std::string const& name)
{
FILE* lessSecret = logging_fopen(name,"r");
ON_BLOCK_EXIT(logging_fclose, lessSecret, name);
... use lessSecret ...
} // lessSecret automagically closed..?
|
The underlying problem here is that logging_fopen is "broken", in a
way. std::fopen "is allowed to" return an unmanaged resource because it
is a C function, but logging_fopen is obviously a C++ function.
ScopeGuard is aimed at making C-style code exception safe.
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
|
Joshua Lehrer
Guest
|
| Posted: Sun Dec 19, 2004 9:57 am Post subject: Re: ScopeGuard template: how to use safely? |
|
|
| Quote: | The underlying problem here is that logging_fopen is "broken", in a
way. std::fopen "is allowed to" return an unmanaged resource because
it
is a C function, but logging_fopen is obviously a C++ function.
ScopeGuard is aimed at making C-style code exception safe.
|
I disagree. ScopeGuard is aimed at making easier the creation of
stack-based resource guards, because if it wasn't easy, nobody would do
it:
"Everybody knows how things must be done by the book, but will
consistently take the shortcut. The one true way is to provide reusable
solutions that are correct and easy to use.
"ScopeGuard is useful when you need to perform automatic cleanup of
resources and can rely on failure-proof undo operations. This idiom is
important when you want to assemble an operation out of several atomic
operations, each of which could fail but can also be undone.
So, ScopeGuard has two major uses:
1: to EASILY make resource guards for non-guarded resources
2: to EASILY string together exception safe operations into one large
exception safe operation
joshua lehrer
factset research systems
NYSE:FDS
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
|
Louis-Philippe Gagnon
Guest
|
| Posted: Sun Dec 19, 2004 9:58 am Post subject: Re: ScopeGuard template: how to use safely? |
|
|
Branimir Maksimovic wrote:
| Quote: | Louis-Philippe Gagnon wrote:
.......
ScopeGuardImpl1(const Fun& fun, const Parm& parm)
: fun_(fun), parm_(parm)
[...]
Fun fun_;
const Parm parm_;
Just put
const Parm& parm_;
// works the same and you don't have copy construction
// that can throw
|
So your answer to this...
Louis-Philippe Gagnon wrote:
| Quote: | - Would ScopeGuard-like classes be better designed if they kept
references to their parameters rather than copies, or would lifetime
issues make the cure worse than the disease?
|
.....would be "yes, it would be better designed"?
Here's another artificial example to illustrate the type of "lifetime
issues" I had in mind:
{
FILE* topSecret = fopen("cia.txt","r");
ON_BLOCK_EXIT(std::fclose, topSecret);
... use topSecret ...
/* the rest of the function needs to see 'topSecret' as
NULL, but the file must remain open, e.g. to keep the file
locked. */
topSecret = NULL;
... more work...
}
Here, a ScopeGuard keeping copies of its parameters will close the
file, but a ScopeGuard keeping references will call fclose(NULL). As
before, there are obvious workarounds, but the default behavior seems
error-prone.
Incidentally, a colleague came up with an interesting idea to close
the exception-safety hole, we haven't had time to explore its
implications fully. The idea is to rewrite the ScopeGuardImpl
constructors like this:
ScopeGuardImpl1(const Fun& fun, const Parm& parm)
try
: fun_(fun), parm_(parm)
{ /* empty ctor body */
} catch (...) {
fun(parm);
/* implicit rethrow */
}
That is, if a parameter's copy-ctor throws, invoke the function using
the original parameters. Some early thoughts on this approach:
- we could run into trouble if one of the parameters had auto_ptr-like
copy semantics, but I think that's pathological enough to ignore
- as a practical matter, the catch/rethrow could reduce the amount of
information a debugger can provide for uncaught exceptions. (I was
surprised to see that visual studio 2003 gets it right)
--
LPG
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
|
Louis-Philippe Gagnon
Guest
|
| Posted: Sun Dec 19, 2004 9:58 am Post subject: Re: ScopeGuard template: how to use safely? |
|
|
Peter Dimov wrote:
| Quote: | The underlying problem here is that logging_fopen is "broken", in a
way. std::fopen "is allowed to" return an unmanaged resource because it
is a C function, but logging_fopen is obviously a C++ function.
ScopeGuard is aimed at making C-style code exception safe.
|
A good point, but not very comforting. Having to deal with "broken"
interfaces is something I have to do every day (my employers prefer
the term "legacy"). Additionally, consider this alternate example:
void log_info(std::string const&);
void broken(std::string const& info)
{
ON_BLOCK_EXIT(&log_info, info);
... do work ...
} // info is logged..?
Should log_info also be considered "broken"?
--
LPG
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
|
Louis-Philippe Gagnon
Guest
|
| Posted: Sun Dec 19, 2004 9:59 am Post subject: Re: ScopeGuard template: how to use safely? |
|
|
Paavo Helde wrote:
| Quote: | To my mind, ON_BLOCK_EXIT is kind of destructor, meaning that it has to
be no-throw.
|
I agree this is true of the function being registered, but should it
be true of the registration operation itself?
| Quote: | If you pass any kind of arguments with throwing copy ctor,
then you violate this silent assumption.
|
This is actually what worries me: the fact that the assumption *is*
silent. This seems to make the idiom more error-prone than I would
like. Otherwise this...
Paavo Helde wrote:
| Quote: | The remedy is simple: if the copy ctor can throw, then avoid it:
FILE* lessSecret = logging_fopen(name,"r");
ON_BLOCK_EXIT(logging_fclose, lessSecret, ByRef(name));
|
.....was my thought as well:
Louis-Philippe Gagnon wrote:
| Quote: | Now, there are workarounds, of course; for example, it looks like the
RefHolder class presented at the end of the article could be used to
avoid the unwanted copy.
|
Paavo Helde also wrote:
| Quote: | Finally, I would not worry too much about filename copy ctor throwing.
IMHO a stack overflow can happen much more likely, leading directly to
undefined behaviour.
|
A point of view I hadn't considered, and with which I'd generally
agree. However I've recently started work on stuffing a win32
application inside a phone; I expect out-of-memory conditions to start
biting me any day now, whereas the app's architecture ensures very
shallow call stacks.
--
LPG
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
|
Doug Harrison
Guest
|
| Posted: Sun Dec 19, 2004 9:59 am Post subject: Re: ScopeGuard template: how to use safely? |
|
|
Branimir Maksimovic wrote:
| Quote: | Louis-Philippe Gagnon wrote:
Nearly 2 years ago, I came across the ScopeGuard CUJ article by
Andrei
Alexandrescu and Petru Marginean [1]. It was my first exposure to the
"true power" of templates, and I haven't looked back since.
That is, until last night, when I realized I was about to use a
scopeguard-like class[2] in a decidedly exception-unsafe way.
.......
ScopeGuardImpl1(const Fun& fun, const Parm& parm)
: fun_(fun), parm_(parm)
[...]
Fun fun_;
const Parm parm_;
Just put
const Parm& parm_;
// works the same and you don't have copy construction
// that can throw
|
With Parm std::string, you could then do things like:
ScopeGuardImpl1 x(whatever, "oops");
I've found lifetime issues introduced by creation from temporaries
frequently make const reference members a bad idea. It's not just
non-explicit ctors you have to worry about; the following is also
wrong, where GetString returns std::string:
ScopeGuardImpl1 x(whatever, GetString());
This all looks quite reasonable, but with parm_ a const reference,
it's all undefined.
--
Doug Harrison
[email]dsh (AT) mvps (DOT) org[/email]
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
|
Branimir Maksimovic
Guest
|
| Posted: Mon Dec 20, 2004 12:43 am Post subject: Re: ScopeGuard template: how to use safely? |
|
|
Doug Harrison wrote:
| Quote: | Branimir Maksimovic wrote:
Just put
const Parm& parm_;
// works the same and you don't have copy construction
// that can throw
With Parm std::string, you could then do things like:
ScopeGuardImpl1 x(whatever, "oops");
|
No, I don't think you'll do this as "oops" can produce temporary
that can throw and then you have resource leak.
| Quote: |
I've found lifetime issues introduced by creation from temporaries
frequently make const reference members a bad idea.
|
That's true but not in this case as we want to prevent
creation of temporary between resource acquisition
and creation of scope guard.
It's not just
| Quote: | non-explicit ctors you have to worry about; the following is also
wrong, where GetString returns std::string:
ScopeGuardImpl1 x(whatever, GetString());
|
Same thing, GetString() can throw and leave resource leak.
| Quote: |
This all looks quite reasonable, but with parm_ a const reference,
it's all undefined.
|
This does not looks reasonable when using scope guard.
Greetings, Bane,
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
|
Paavo Helde
Guest
|
| Posted: Mon Dec 20, 2004 12:51 am Post subject: Re: ScopeGuard template: how to use safely? |
|
|
Louis-Philippe Gagnon <lp_gagnon (AT) videotron (DOT) ca> wrote in
news:asb9s01vufoebr602ogtmnqovpclhefpmq (AT) 4ax (DOT) com:
| Quote: | Paavo Helde wrote:
To my mind, ON_BLOCK_EXIT is kind of destructor, meaning that it has to
be no-throw.
I agree this is true of the function being registered, but should it
be true of the registration operation itself?
|
No, it shouldn't, this is just an unfortunate side-effect of the most
convenient usage of the ScopeGuard idiom. One could prepare the
registration operation beforehand, assuming that logging_fclose() is a
non-op for NULL filepointer:
FILE* lessSecret=NULL;
ON_BLOCK_EXIT(logging_fclose, ByRef(lessSecret), name);
lessSecret = logging_fopen(name, "r");
Here also ByRef() is needed, which can also be quite easily forgotten,
resulting in the same bug as before (file not closed properly), so I
think this is not a real improvement.
I think the bottom line is that in C++ you always have to know what you
are doing. Either implement your own exception-safe classes, or find out
what your tools are exactly doing. Yes I agree this gotcha should have
been discussed more explicitly in the ScopeGuard documentation.
Regards
Paavo
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
|
Branimir Maksimovic
Guest
|
| Posted: Mon Dec 20, 2004 12:52 am Post subject: Re: ScopeGuard template: how to use safely? |
|
|
Louis-Philippe Gagnon wrote:
| Quote: | Branimir Maksimovic wrote:
Louis-Philippe Gagnon wrote:
.......
ScopeGuardImpl1(const Fun& fun, const Parm& parm)
: fun_(fun), parm_(parm)
[...]
Fun fun_;
const Parm parm_;
Just put
const Parm& parm_;
// works the same and you don't have copy construction
// that can throw
So your answer to this...
Louis-Philippe Gagnon wrote:
- Would ScopeGuard-like classes be better designed if they kept
references to their parameters rather than copies, or would lifetime
issues make the cure worse than the disease?
....would be "yes, it would be better designed"?
|
Of course.
Same case as
class AutoLock{
public:
AutoLock(Lock& lock):lock_(lock)
{
lock_.acquire();
}
~AutoLock()
{
lock_.release();
}
private:
Lock& lock_;
};
| Quote: |
Here's another artificial example to illustrate the type of "lifetime
issues" I had in mind:
{
FILE* topSecret = fopen("cia.txt","r");
ON_BLOCK_EXIT(std::fclose, topSecret);
... use topSecret ...
/* the rest of the function needs to see 'topSecret' as
NULL, but the file must remain open, e.g. to keep the file
locked. */
topSecret = NULL;
... more work...
}
|
Nope, that example is not good as you wouldn't write such code.
realistical would be:
FILE* topSecret = fopen("cia.txt","r");
FILE* tmpSecret = topSecret; // you have to save pointer
// to file or you can't close file at all
topSecret = 0;
// ... code ...
fclose(tmpSecret);
// or
// topSecret = tmpSecret;
// fclose(topSecret);
| Quote: |
Here, a ScopeGuard keeping copies of its parameters will close the
file, but a ScopeGuard keeping references will call fclose(NULL). As
before, there are obvious workarounds, but the default behavior seems
error-prone.
|
I can't agree as you exactly know what happens
in this case.
| Quote: |
Incidentally, a colleague came up with an interesting idea to close
the exception-safety hole, we haven't had time to explore its
implications fully. The idea is to rewrite the ScopeGuardImpl
constructors like this:
ScopeGuardImpl1(const Fun& fun, const Parm& parm)
try
: fun_(fun), parm_(parm)
{ /* empty ctor body */
} catch (...) {
fun(parm);
/* implicit rethrow */
}
That is, if a parameter's copy-ctor throws, invoke the function using
the original parameters.
|
This approach defeats the purpose of ON_BLOCK_EXIT.
Your code will not execute, that is, you will close file
unecesseraly, just because you use scope guard.
Greetings, Bane.
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
|
Doug Harrison
Guest
|
| Posted: Mon Dec 20, 2004 10:38 am Post subject: Re: ScopeGuard template: how to use safely? |
|
|
Branimir Maksimovic wrote:
| Quote: | Doug Harrison wrote:
I've found lifetime issues introduced by creation from temporaries
frequently make const reference members a bad idea.
That's true but not in this case as we want to prevent
creation of temporary between resource acquisition
and creation of scope guard.
|
The suggestion to replace a value member with a const reference member
does not accomplish that. It seems to me it continues to permit it and
introduces a new lifetime issue, which in practice, is a lot more
likely to bite than string's copy ctor throwing an exception.
Avoidance of each of these issues relies on programmer discipline,
except the temporary binding issue is a lot more subtle.
| Quote: | It's not just
non-explicit ctors you have to worry about; the following is also
wrong, where GetString returns std::string:
ScopeGuardImpl1 x(whatever, GetString());
Same thing, GetString() can throw and leave resource leak.
|
The problem is, it compiles just as well as a function that returns
const string&, for which the binding may be valid, or in the OP's
case, a function argument, for which the binding would be valid, and
which may have been all there was when the code was written. Then
later someone comes along and innocently does something that creates a
temporary...
| Quote: | This all looks quite reasonable, but with parm_ a const reference,
it's all undefined.
This does not looks reasonable when using scope guard.
|
OK, but I was focusing on the suggestion to replace a value member
with a const reference member. The point is, if you bind function
arguments to const reference members, you may open yourself up to the
subtle lifetime issues I described. It's an easy mistake to make.
--
Doug Harrison
[email]dsh (AT) mvps (DOT) org[/email]
[ See http://www.gotw.ca/resources/clcm.htm for info about ]
[ comp.lang.c++.moderated. First time posters: Do this! ]
|
|
| Back to top |
|
|
|
|
You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum
|
|
FAQ
Search
Memberlist
Usergroups
Register
Profile
Log in to check your private messages
Log in
