Dear Doug Cook,

     The version that I believe properly releases the resources and
works has been implemented (https://github.com/soasis/out_ptr) and the
fix has been put up in the issue
(https://cplusplus.github.io/LWG/issue3594). In a couple of days I'll
ping LWG to ask if it's ready to move (or what the procedure is to get
it ready to move into the current Working Draft). Once it moves into
the Working Draft and all parties are happy, I'll ping the one
implementation that already has it implemented for C++23 (MSVC'S STL,
I believe) and go from there.

     Again, thanks for letting me know!

Sincerely,
JeanHeyd

On Sat, Sep 18, 2021 at 4:03 PM JeanHeyd Meneide
<phdofthehouse_at_[hidden]> wrote:
>
> Dear Doug Cook,
>
> An issue has been filed
> (https://cplusplus.github.io/LWG/issue3594). I think the suggested
> wording fix covers the issue you are facing, and so when this goes up
> for prioritization in the Library Working/Wording Group (LWG), it
> might be labeled "P0" and then bundled as "fixes to apply the next
> time we have a virtual plenary meeting", which might be either at the
> end of this year or the first plenary next year. Once the issue is
> resolved (it's status will be updated on that page to CLOSED -
> FIXED/RESOLVED) you can then go run around to implementers and tell
> them to fix it, but if you're maintaining your own implementation
> (like me) you can just do the fix early.
>
> Thanks for reaching out!
>
> Sincerely,
> JeanHeyd
>
> On Tue, Aug 3, 2021 at 4:35 PM Doug Cook (WINDOWS) <dcook_at_[hidden]> wrote:
> >
> > I appreciate your response and the thought going into this.
> >
> >
> >
> > I might be missing something, but I don’t think unconditional release leads to leaks or other incorrect behavior. The unconditional release is followed by a reset so that nothing is leaked. This may be sub-optimal, but it is not incorrect behavior.
> >
> >
> >
> > Again, expanding the call to inout_ptr so we can see what it is doing:
> >
> >
> >
> > auto tempPtr = smartPtr.get();
> >
> > MyFunc(&tempPtr);
> >
> > smartPtr.release(); // Unconditional. Before or after MyFunc isn’t important.
> >
> > If (tempPtr)
> >
> > {
> >
> > smartPtr.reset(tempPtr);
> >
> > }
> >
> >
> >
> > This is fine for both (1) and (2).
> >
> >
> >
> > - In case (1) failure, we call smartPtr.release(), which is fine because MyFunc deleted the value, and then skip the condition (no reset), which is fine because that leaves smartPtr set to the value returned by MyFunc (NULL).
> >
> >
> >
> > - In case (2) failure, we call smartPtr.release(), which is unnecessary because MyFunc did not delete the value, but we then execute the reset, so the smartPtr re-acquires ownership and we’re back in a consistent state.
> >
> >
> >
> > From my perspective as a developer, the primary guarantees I want are that, unless an exception occurs, smartPtr always receives the value returned by MyFunc (even if that value is NULL) and that regardless of exception, resource ownership is properly managed.
> >
> >
> >
> > From this point of view, the IF statement in the destructor is purely an optimization. The general behavior of out_ptr should model:
> >
> >
> >
> > T* tempPtr = nullptr;
> >
> > MyFunc(&tempPtr);
> >
> > smartPtr.reset(tempPtr);
> >
> >
> >
> > And the general behavior of inout_ptr should model:
> >
> >
> >
> > T* tempPtr = smartPtr.get();
> >
> > MyFunc(&tempPtr);
> >
> > smartPtr.release();
> >
> > smartPtr.reset(tempPtr);
> >
> >
> >
> > Additional IF statements can be added for optimization purposes but should not affect the general behavior. For example, if we are concerned about the costs of reset, then out_ptr could be optimized as
> >
> >
> >
> > T* tempPtr = nullptr;
> >
> > MyFunc(&tempPtr);
> >
> > If (tempPtr)
> >
> > smartPtr.reset(tempPtr);
> >
> > else
> >
> > smartPtr.reset();
> >
> >
> >
> > And inout_ptr could be optimized as
> >
> >
> >
> > T* tempPtr = smartPtr.get();
> >
> > MyFunc(&tempPtr);
> >
> > If (tempPtr != smartPtr.get())
> >
> > {
> >
> > smartPtr.release();
> >
> > if (tempPtr)
> >
> > smartPtr.reset(tempPtr);
> >
> > }
> >
> >
> >
> > Thanks!
> >
> > Doug
> >
> >
> >
> > From: JeanHeyd Meneide
> > Sent: Tuesday, August 3, 2021 12:02 PM
> > To: std-discussion_at_[hidden]
> > Cc: Doug Cook (WINDOWS)
> > Subject: [EXTERNAL] Re: [std-discussion] inout_ptr - inconsistent release()
> >
> >
> >
> > (Woops, a follow-up because I didn't translate the example code
> > correctly for (1) and (2). Here it is, better:
> >
> > // Implementation (1)
> > void MyFunc(BYTE** p_bytes) {
> > delete(*p_bytes);
> > BYTE* new_ptr = new BYTE[42]();
> > if (new_ptr) {
> > *p_bytes = new_ptr;
> > }
> > else {
> > *p_bytes = NULL;
> > }
> > }
> >
> > // Implementation (2)
> > void MyFunc(BYTE** p_bytes) {
> > BYTE* new_ptr = new BYTE[42]();
> > if (new_ptr) {
> > delete(*p_bytes);
> > *p_bytes = new_ptr;
> > }
> > else {
> > // literally nothing
> > }
> > }
> >
> > Also, note that an implementation of (1) can have better exception
> > safety, if you write it AFTER the coffee kicks in and not before:
> >
> > // Implementation (1a)
> > void MyFunc(BYTE** p_bytes) {
> > delete(*p_bytes);
> > *p_bytes = NULL;
> > BYTE* new_ptr = new BYTE[42]();
> > if (new_ptr) {
> > *p_bytes = new_ptr;
> > }
> > }
> >
> > So there's still a lot to fix here...)
> >
> > On Tue, Aug 3, 2021 at 1:55 PM JeanHeyd Meneide <phdofthehouse_at_[hidden]> wrote:
> > >
> > > There's a lot of waxing poetical about C API designs below, skip to
> > > the "-------------------------------" if you want to get to what a
> > > more effective change would be.
> > >
> > > Reading this over, I think at the core there's effectively 2 different
> > > styles of C API that take in-and-(maybe-)out pointers, where the
> > > "release always" approach is either good or bad in a mutually
> > > exclusive way. Let's elaborate using 2 implementations of MyFunc, one
> > > that shows the behavior you have here and the other that is the kind I
> > > (one of the proposal authors for out_ptr/inout_ptr) encountered more
> > > often (always?) in the wild:
> > >
> > > // Implementation (1)
> > > void MyFunc(BYTE** p_bytes) {
> > > free(*ptr);
> > > BYTE* new_ptr = new BYTE[42]();
> > > if (new_ptr) {
> > > *p_bytes = new_ptr;
> > > }
> > > else {
> > > *p_bytes = new_ptr;
> > > }
> > > }
> > >
> > > // Implementation (2)
> > > void MyFunc(BYTE** p_bytes) {
> > > BYTE* new_ptr = new BYTE[42]();
> > > if (new_ptr) {
> > > delete(*ptr);
> > > *p_bytes = new_ptr;
> > > }
> > > else {
> > > // literally nothing
> > > }
> > > }
> > >
> > > Let us consider always calling .release(), unconditionally, in the
> > > constructor or the destructor of an inout_ptr_t. Here are the 2 things
> > > that happen:
> > >
> > > — For (1), it works as outlined in your post. There is no double-free,
> > > because the pointer is unconditionally deleted by the C function. This
> > > is good and solves the presented problem.
> > >
> > > — For (2), it works to the opposite. This C API does not do anything
> > > with the input if it cannot guarantee that it can replace the value.
> > > Calling .release() unconditionally here results in the opposite bug of
> > > a dangling resource.
> > >
> > > What complicates matters more is that both approaches are very valid
> > > to designing a C API. For example, (1) is a good implementation for
> > > certain scenarios and for resource-constrained environments. Deleting
> > > the resource always means that, in a resource-constrained environment,
> > > you have a greater chance that the next re-creation of the resource is
> > > more successful. This does, unfortunately, require making sure that
> > > there is some sentinel value / error value communicated to the user
> > > about this decision (e.g., setting the pointer to NULL).
> > >
> > > (2) is a good implementation for systems meant to be more "inert" or
> > > "atomic" in their changes. It guarantees that nothing happens and
> > > everything is rolled back if it cannot guarantee that the replacement
> > > happens. Either the in-out portion of the operation happens, or it
> > > happens not at all. (This also matches the ideals behind exception
> > > safety: for example, if "new" throws in either (1) or (2), the (2)
> > > implementation is safer. In a C Environment, that's never supposed to
> > > happen, but there are many implementations which
> > > wink-wink-hint-hint-nudge-nudge and let you throw an exception or two
> > > through C API calls anyways.)
> > >
> > > -------------------------------
> > > -------------------------------
> > >
> > > Given that making these changes effectively throws one design under
> > > the bus, I think this might be evidence that one of a few things might
> > > need to happen (and I'd be willing to do the legwork to make sure this
> > > happens, since it was my effort to put it in the standard after all!)
> > >
> > > A. Add additional tracking to std::inout_ptr. This means holding onto
> > > the "old" value and, instead of comparing against "nullptr", comparing
> > > against the old value and letting that be the trigger for calling
> > > .reset() (or equivalent). This covers (should cover?) the
> > > always-delete case AND the delete-and-reset-or-do-nothing case. This
> > > comes at the cost of additional performance, which I am none too sure
> > > some of the original instigators of this proposal would be happy about
> > > since one of the primary reasons was putting it in the standard so the
> > > standard pointers had a chance to "directly reseat the pointer". I
> > > need to think more about whether the direct re-seating optimization is
> > > unaffected by this, and run some more tests after the coffee hits.
> > >
> > > B. Create a third type that carefully captures the semantic difference
> > > between "I unconditionally delete my pointers" and "I only
> > > conditionally delete if the operation I am doing is a success". I'd
> > > note the nomenclature for this might actually already be in place:
> > > out_ptr only handles cases where a pointer is coming out. inout_ptr
> > > only handles the cases where a pointer goes in AND comes out changed.
> > > in_ptr would handle a pointer that goes in and is unconditionally
> > > deleted, and only MAYBE comes out the other side. (I think people
> > > would object to this kind of framing and the name in_ptr, but it is a
> > > start. Perhaps the name could be in_maybe_out_ptr, or something
> > > equally verbose). This approach retains performance no matter what,
> > > but puts the burden of knowing the "kind" of C API on the end-user.
> > >
> > > C. (This one needs to happen regardless.) I need to try to refine the
> > > wording around how .release() is called in the constructor. The ideal
> > > was to prevent the user from observing that .release() is called until
> > > the destructor happens, effectively leaving the state of the original
> > > smart pointer in flux until after the destructor.
> > >
> > > I'm not sure which path of (A) or (B) is best. Given the example above
> > > the marquee lines, I think that there are 2 different semantics here
> > > and I am unsure of how mixing both kinds in inout_ptr may impact
> > > speed. (It ultimately sounds like a better user experience, because
> > > someone having to know what very well may be a secret internal
> > > implementation detail to differentiate between one of 3 different
> > > out_ptr / inout_ptr / in{something}_ptr is... tough.)
> > >
> > > In either case, thank you for catching this, Doug!! I've got some
> > > implementation and options to start benchmarking and testing, and some
> > > people to e-mail! I'll make sure this gets fixed before ship.
> > >
> > > Sincerely,
> > > JeanHeyd
> >
> >