[ptr_container] Questionable strong guarantee of ptr_map::insert()
Hi, With current implementation of ptr_map::insert(), if an exception is thrown, the objected pointed by the passed pointer is deleted. Is this really providing strong guarantee as documented? I understand strong guarantee as described here. http://www.boost.org/community/exception_safety.html
The strong guarantee: that the operation has either completed successfully or thrown an exception, leaving the program state exactly as it was before the operation started. I think ptr_map::insert() doesn't provide this guarantee because deleting the object is a change of the program state.
As more concrete problem, see the attached code. I couldn't find any way to provide strong guarantee for delayed_inserter::insert_ready() using ptr_map. I want ready_value unchanged if an exception is thrown during the insertion. Looking for a solution on a similar situation, I found constructors of shared_ptr. Its documentation clearly says that "If an exception is thrown, delete p is called". And for auto_ptr version, it provides strong guarantee by taking the auto_ptr as non-const reference. I think ptr_map::insert() should follow this manner; document the deletion on exception for raw pointer version, and provide strong guarantee with insert(const key_type& key, std::auto_ptr<U>& x) . The the auto_ptr& version can be a better replacement of the raw pointer version. The required temporary of auto_ptr, instead of key_type, is more expressive (about the purpose of the temporary), and matches the Best Practices shown in the documentation of shared_ptr. http://www.boost.org/libs/smart_ptr/shared_ptr.htm#BestPractices But, introducing the auto_ptr& version seems to cause some change of behavior in client codes using the current pass-by-value version. Is it valid to request (create a ticket) for strong guaranteed (taking auto_ptr&) version while having this compatibility concern? Regards. -- k_satoda #include <memory> #include "boost/ptr_container/ptr_map.hpp" namespace ptr_map_insert_strong_guarantee_demo { class value { public: explicit value(int source); // ... }; class delayed_inserter { public: // ... // This is called from context that knows key and source of value, // but doesn't know the time of insertion. void ready_new(int key, int source) { // OK. // This implementation offers strong guarantee for this call. std::auto_ptr<value> new_value(new value(source)); ready_key = key; ready_value = new_value; } // This is called from context which doesn't know key and value, // but knows the time of insertion. void insert_ready() { if (ready_value.get() != 0) { // I want to offer strong guarantee for this call, // but boost::ptr_map deosn't provide the way to do that; // *** ready_value is deleted when an exception is thrown. *** map.insert(ready_key, ready_value); } } // ... private: int ready_key; std::auto_ptr<value> ready_value; boost::ptr_map<int, value> map; // ... }; }
on Mon Aug 17 2009, Kazutoshi Satoda <k_satoda-AT-f2.dion.ne.jp> wrote:
Hi,
With current implementation of ptr_map::insert(), if an exception is thrown, the objected pointed by the passed pointer is deleted. Is this really providing strong guarantee as documented?
I understand strong guarantee as described here. http://www.boost.org/community/exception_safety.html
The strong guarantee: that the operation has either completed successfully or thrown an exception, leaving the program state exactly as it was before the operation started. I think ptr_map::insert() doesn't provide this guarantee because deleting the object is a change of the program state.
You are correct. I think the implementation of ptr_map::insert is probably as correct as can be in doing that deletion (i.e. the docs are wrong, not the implementation), but if I had written ptr_map I'd have given it an auto_ptr parameter instead. -- Dave Abrahams BoostPro Computing http://www.boostpro.com
AMDG David Abrahams wrote:
I think the implementation of ptr_map::insert is probably as correct as can be in doing that deletion (i.e. the docs are wrong, not the implementation), but if I had written ptr_map I'd have given it an auto_ptr parameter instead.
There is an overload that takes an auto_ptr. In Christ, Steven Watanabe
Thanks for the follow ups. Now I noticed that the problem is not only in ptr_map::insert(). The same problem seems wide spread in the whole ptr_container library. I'll make the following 2 tickets in boost trac if nobody objects nor gives better suggestion. 1. (Type: Bugs) False strong guarantee in ptr_container documentation 2. (Type: Feature Requests) Provide strong guaranteed overloads in ptr_container Steven Watanabe wrote:
David Abrahams wrote:
I think the implementation of ptr_map::insert is probably as correct as can be in doing that deletion (i.e. the docs are wrong, not the implementation), but if I had written ptr_map I'd have given it an auto_ptr parameter instead.
There is an overload that takes an auto_ptr.
Yes, but unfortunately, it doesn't provide strong guarantee, as same as the raw pointer version. And also, the current auto_ptr pass-by-value version can be a error-prone. An casual user of ptr_map will likely write a code at first like this: map.insert(some_key(...), new value(...)); This doesn't compile by design. Then the user will look for the docs to find alternative ways, find the auto_ptr version, and modify the code to shut off the strange compile error, like this: map.insert(some_key(...), std::auto_ptr<value>(new value)); This does compile, with the risk of subtle memory leaks. But the user can hardly notice the problem, and would think, "Aha, I can guess, the use of auto_ptr is the fix for exception-safety problem which the FAQ mentions. The library taught me that with the error. So nice." -- k_satoda
Kazutoshi Satoda skrev:
Thanks for the follow ups.
Now I noticed that the problem is not only in ptr_map::insert(). The same problem seems wide spread in the whole ptr_container library.
I'll make the following 2 tickets in boost trac if nobody objects nor gives better suggestion.
1. (Type: Bugs) False strong guarantee in ptr_container documentation
Right, it should be replaced with the basic guarantee and a note saying the object is deleted when an exception is thrown.
2. (Type: Feature Requests) Provide strong guaranteed overloads in ptr_container
Hm. Do you propose to break the existing interface?
Steven Watanabe wrote:
David Abrahams wrote:
I think the implementation of ptr_map::insert is probably as correct as can be in doing that deletion (i.e. the docs are wrong, not the implementation), but if I had written ptr_map I'd have given it an auto_ptr parameter instead.
There is an overload that takes an auto_ptr.
Yes, but unfortunately, it doesn't provide strong guarantee, as same as the raw pointer version.
And also, the current auto_ptr pass-by-value version can be a error-prone.
An casual user of ptr_map will likely write a code at first like this:
map.insert(some_key(...), new value(...));
This doesn't compile by design. Then the user will look for the docs to find alternative ways, find the auto_ptr version, and modify the code to shut off the strange compile error, like this:
map.insert(some_key(...), std::auto_ptr<value>(new value));
This does compile, with the risk of subtle memory leaks. But the user can hardly notice the problem, and would think, "Aha, I can guess, the use of auto_ptr is the fix for exception-safety problem which the FAQ mentions. The library taught me that with the error. So nice."
People need to initialize smart pointers in seperate statements. -Thorsten
Thorsten Ottosen wrote:
Kazutoshi Satoda skrev:
1. (Type: Bugs) False strong guarantee in ptr_container documentation
Right, it should be replaced with the basic guarantee and a note saying the object is deleted when an exception is thrown.
Putting "basic guarantee" seems over relaxing. One should be able to assume the contents of the container are unchanged when an exception is thrown. I propose the wording like this; "When an exception is thrown, nothing happens except performing delete x."
2. (Type: Feature Requests) Provide strong guaranteed overloads in ptr_container
Hm. Do you propose to break the existing interface?
Yes, I personally believe that the following use should be banned by design.
map.insert(some_key(...), std::auto_ptr<value>(new value));
And I agree with this practice.
People need to initialize smart pointers in seperate statements.
I want to hear about some compatibility policy in Boost to guess the possibility of this kind of breaking (but otherwise good) changes. -- k_satoda
Kazutoshi Satoda skrev:
Thorsten Ottosen wrote:
Kazutoshi Satoda skrev:
1. (Type: Bugs) False strong guarantee in ptr_container documentation
Right, it should be replaced with the basic guarantee and a note saying the object is deleted when an exception is thrown.
Putting "basic guarantee" seems over relaxing. One should be able to assume the contents of the container are unchanged when an exception is thrown.
I propose the wording like this; "When an exception is thrown, nothing happens except performing delete x."
2. (Type: Feature Requests) Provide strong guaranteed overloads in ptr_container
Hm. Do you propose to break the existing interface?
Yes, I personally believe that the following use should be banned by design.
map.insert(some_key(...), std::auto_ptr<value>(new value));
And I agree with this practice.
People need to initialize smart pointers in seperate statements.
I want to hear about some compatibility policy in Boost to guess the possibility of this kind of breaking (but otherwise good) changes.
Well, you also break perfectly valid and safe code where the auto pointer is returned from a function. What we could do instead is to either overload (if it does not lead to ambiguities) or provide new versions of strong_push_back() strong_push_front() strong_insert() ... with the strong guarantee. My tests show that overloading does lead to ambiguities. So we need to add new functions. -Thorsten
2009/8/24 Thorsten Ottosen <thorsten.ottosen@dezide.com>
I want to hear about some compatibility policy in Boost to guess the
possibility of this kind of breaking (but otherwise good) changes.
Well, you also break perfectly valid and safe code where the auto pointer is returned from a function.
While it is a breaking change, it is a compile-time breaking change with a very easy fix for the caller (they pass FunctionReturningAutoPtr().release() instead of FunctionReturningAutoPtr()). And it isn't like the library hasn't had breaking changes before (the iterators had changed). Currently, the semantics are surprising (they certainly are to me); I believe that most people expect that if they pass an auto_ptr and an exception is thrown, then the auto_ptr has not relinquished ownership. Otherwise, why bother with an auto_ptr at all? -- Nevin ":-)" Liber <mailto:nevin@eviloverlord.com> (847) 691-1404 Sent from Chicago, Illinois, United States
Nevin ":-]" Liber wrote:
2009/8/24 Thorsten Ottosen <thorsten.ottosen@dezide.com>
I want to hear about some compatibility policy in Boost to guess the
possibility of this kind of breaking (but otherwise good) changes.
Well, you also break perfectly valid and safe code where the auto pointer is returned from a function.
While it is a breaking change, it is a compile-time breaking change with a very easy fix for the caller (they pass FunctionReturningAutoPtr().release() instead of FunctionReturningAutoPtr()). And it isn't like the library hasn't had breaking changes before (the iterators had changed). Currently, the semantics are surprising (they certainly are to me); I believe that most people expect that if they pass an auto_ptr and an exception is thrown, then the auto_ptr has not relinquished ownership. Otherwise, why bother with an auto_ptr at all?
I would have assumed that if I pass an auto_ptr that ownership is relinquished no matter what happens later, exception or not. Is not that how the copy constructor of an auto_ptr is supposed to work ?
Edward Diener wrote:
Nevin ":-]" Liber wrote:
I believe that most people expect that if they pass an auto_ptr and an exception is thrown, then the auto_ptr has not relinquished ownership. Otherwise, why bother with an auto_ptr at all?
I would have assumed that if I pass an auto_ptr that ownership is relinquished no matter what happens later, exception or not. Is not that how the copy constructor of an auto_ptr is supposed to work ?
I agree -- auto_ptr's copy constructor has move semantics, and the copy (move) must be made before the function is entered (arguments can be evaluated in any order, but there's a sequence point before entering the function body). For the record, if the argument was an auto_ptr&, I would expect the auto_ptr to still own the resource if an exception was thrown, although I don't know how to implement it (moving back into the auto_ptr might throw too). --Jeffrey Bosboom
2009/8/24 Jeffrey Bosboom <jbosboom@uci.edu>
For the record, if the argument was an auto_ptr&, I would expect the auto_ptr to still own the resource if an exception was thrown, although I don't know how to implement it (moving back into the auto_ptr might throw too).
The implementation is actually fairly simple. In psuedocode: internal_insert(Iterator position, T* p) { // insert p into the container; throw on failure } insert(Iterator position, auto_ptr<T>& ap) { internal_insert(position, ap.get()); ap.release(); } insert(Iterator position, T* p) { auto_ptr<T> ap(p); insert(position, ap); } While I do understand how "copy" semantics work for auto_ptrs, when I see an auto_ptr as a (sink) parameter to a function, my expectation is that either the callee takes ownership of the underlying object, or if it fails, that I as the caller retain ownership. The details are, of course, that one passes the auto_ptr by non-const reference to any function that might fail. -- Nevin ":-)" Liber <mailto:nevin@eviloverlord.com> (847) 691-1404
Nevin ":-]" Liber wrote:
2009/8/24 Jeffrey Bosboom <jbosboom@uci.edu>
For the record, if the argument was an auto_ptr&, I would expect the auto_ptr to still own the resource if an exception was thrown, although I don't know how to implement it (moving back into the auto_ptr might throw too).
The implementation is actually fairly simple. In psuedocode:
internal_insert(Iterator position, T* p) { // insert p into the container; throw on failure }
insert(Iterator position, auto_ptr<T>& ap) { internal_insert(position, ap.get()); ap.release(); }
insert(Iterator position, T* p) { auto_ptr<T> ap(p); insert(position, ap); }
Reasonable
While I do understand how "copy" semantics work for auto_ptrs, when I see an auto_ptr as a (sink) parameter to a function, my expectation is that either the callee takes ownership of the underlying object, or if it fails, that I as the caller retain ownership. The details are, of course, that one passes the auto_ptr by non-const reference to any function that might fail.
You might argue that the function should take an auto_ptr by reference, but as written, it does what it ought. _____ Rob Stewart robert.stewart@sig.com Software Engineer, Core Software using std::disclaimer; Susquehanna International Group, LLP http://www.sig.com IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.
AMDG Jeffrey Bosboom wrote:
For the record, if the argument was an auto_ptr&, I would expect the auto_ptr to still own the resource if an exception was thrown, although I don't know how to implement it (moving back into the auto_ptr might throw too).
auto_ptr operations never throw. In Christ, Steven Watanabe
Steven Watanabe wrote:
AMDG
Jeffrey Bosboom wrote:
For the record, if the argument was an auto_ptr&, I would expect the auto_ptr to still own the resource if an exception was thrown, although I don't know how to implement it (moving back into the auto_ptr might throw too).
auto_ptr operations never throw.
Oh. Duh. That's what I get for trying to talk about auto_ptr having not used it for years. Thank you for correcting my ignorance. --Jeffrey Bosboom
Thorsten Ottosen wrote:
Hm. Do you propose to break the existing interface?
map.insert(some_key(...), std::auto_ptr<value>(new value)); (snip) I want to hear about some compatibility policy in Boost to guess the
Yes, I personally believe that the following use should be banned by design. possibility of this kind of breaking (but otherwise good) changes.
Well, you also break perfectly valid and safe code where the auto pointer is returned from a function.
Hmm, I didn't think of such code. I agree such code is valid and safe. However, as far as I know, there is no way to distinguish the two; a temporary auto_ptr, and a returned auto_ptr. Then the interface can allow both, or disallow both. I still think the latter is better choice in a generic library. Do you think passing a temporary auto_ptr should be allowed to allow passing a returned auto_ptr? -- k_satoda
Kazutoshi Satoda skrev:
Thorsten Ottosen wrote:
Hm. Do you propose to break the existing interface?
map.insert(some_key(...), std::auto_ptr<value>(new value)); (snip) I want to hear about some compatibility policy in Boost to guess the
Yes, I personally believe that the following use should be banned by design. possibility of this kind of breaking (but otherwise good) changes.
Well, you also break perfectly valid and safe code where the auto pointer is returned from a function.
Hmm, I didn't think of such code. I agree such code is valid and safe.
However, as far as I know, there is no way to distinguish the two; a temporary auto_ptr, and a returned auto_ptr. Then the interface can allow both, or disallow both. I still think the latter is better choice in a generic library.
Do you think passing a temporary auto_ptr should be allowed to allow passing a returned auto_ptr?
Oh yes. Generic libraries in particular needs to be useable rather than over-encapsulated. If there is a real need for operations with the strong guarantee, I think we should add them with the following signature: strong_push_back( std::auto_ptr<U>&, ... ); albeit they can be implemeted by a user because you have access to the underlying container with .base(): std::auto_ptr<U> ptr( ... ); cont.base().push_back( ptr.get() ); ptr.release(); -Thorsten
Thorsten Ottosen wrote:
Kazutoshi Satoda skrev:
Do you think passing a temporary auto_ptr should be allowed to allow passing a returned auto_ptr?
Oh yes. Generic libraries in particular needs to be useable rather than over-encapsulated.
(Sorry for this late response.) I understood dropping pass-by-value version will decrease the usability in the case where returning auto_ptr from a function and passing another argument as rvalue. But I think it can be justified for the exception-safety as more important aspect, as the FAQ currently says for raw-pointer versions. On the other hand, I don't see what the problem is about the term "over-encapsulated". Could you please explain more about the concrete problem with it?
If there is a real need for operations with the strong guarantee, I think we should add them with the following signature:
strong_push_back( std::auto_ptr<U>&, ... );
albeit they can be implemeted by a user because you have access to the underlying container with .base():
std::auto_ptr<U> ptr( ... ); cont.base().push_back( ptr.get() ); ptr.release();
I didn't know about the base() function. My original motivation can be satisfied with the latter workaround. Thank you. However, I still think providing the strong guaranteed version as the default is better in general. For function calls passing a single argument like push_back(), calling like push_back(returning_auto_ptr(...).release()) seems no-less usable and more expressive about what is happening. -- k_satoda
Kazutoshi Satoda wrote:
1. (Type: Bugs) False strong guarantee in ptr_container documentation
Created the ticket for this one in trac. https://svn.boost.org/trac/boost/ticket/3406 -- k_satoda
participants (8)
-
David Abrahams -
Edward Diener -
Jeffrey Bosboom -
Kazutoshi Satoda -
Nevin ":-]" Liber -
Steven Watanabe -
Stewart, Robert -
Thorsten Ottosen