Re: leaked ODBC statement handles

I am sorry for the length.  I hope some of it is interesting.

On Tue, 2012-07-17 at 19:08 +0200, Lionel Elie Mamane wrote:
> On Mon, Jul 16, 2012 at 10:15:02PM -0400, Terrence Enger wrote:
>
> > I am chasing some "leaked" ODBC statement handles.
>
> > I see that ODatabaseMetaDataResultSet.cxx takes care *not* to free a
> > statement handle which has not been subjected to one of 13 member
> > functions with names starting "open...".  Questions arising ...
>
> That is a bug. For the history, look at commit
> aa3f42832756b7165a0722b2d013a572acf224c8
> http://cgit.freedesktop.org/libreoffice/core/commit/?id=aa3f42832756b7165a0722b2d013a572acf224c8

Thank you for the link.  Having that code brought together--no need to
flip between windows or applications or to flip over intervening
printed pages--quite changes the way I see it.

> Before that commit the statement handle was passed to the constructor;
> the class had not allocated the handle, and thus it was not the
> class's role to free it .... (I guess) unless it had taken ownership
> of it by actually using it. I'm muddy on how the calling code was
> supposed to "know" it should free it "because
> ODatabaseMetaDataResultSet did not use it" or it was supposed *not* to

Yeah, I was looking at this yesterday, and the wee hours of this
morning, and since I got up today.  "Muddy" is the right word for the
way I was feeling before I read your answer.

> free it "because ODBMetaDataRS used it"; I can easily believe the code
> was leaking statement handles in this way even back then (or

Well, I can demonstrate at least a leak.  Removing m_bCloseHandle will
of course fix the leak.  Still, I wonder if the leak could be a sign
of a bug is client code somewhere.  Thoughts?

> double-freeing them or whatever).

Well, yes.  I can imagine (but have not yet tried to demonstrate) this
happening:

(*) Client code calls close, which calls dispose, which frees the
    statement handle

(*) Client code calls dispose, which frees the statement handle a
    second time.  I have not looked at the Reference class since the
    days of OO.o, but IIRC it calls dispose of the referenced class.

(*) The destructor calls dispose, which frees the statement handle a
    third time..

> Anyway, since this commit, the handle is allocated privately "fresh"
> by the class, and it should thus unconditionally free it on dispose;
> m_bFreeHandle should die.
>
> To you the honours of writing the patch? Put me as a reviewer, I'll
> apply it.

Oh, goody.  It is months since I submitted a patch to Base.  I think,
moreover, that I should not wait until I understand more deeply what
is going on.  

I had already decided that m_bCloseHandle in the class definition
needs a comment "this is not what the name suggests", but I had real
trouble deciding what the variable *does* mean.


Removing m_bCloseHandle from ODatabaseMetaDataResultSet clears away
many questions that I was accumulating, and it makes somewhat
plausible my guess that the protocol for using
ODatabaseMetaDataResultSet is something like this:

    construct
    exactly one open<whatever>
    in any order
     |  one or more get<whatever>
     |  zero or more calls repositioning the result set
     |  zero or more calls querying position of result set
    exactly one close
    exactly one dispose
    destruct

You caught me in the middle of thinking about question and writing
code in ODatabaseMetaDataResultSet.cxx to shout out if the guess is
false.  Do you have a feeling for whether this guess is plausible?


If the guess is plausible, should the class complain (SAL_WARN_IF, I
presume) about violations of the protocol?  If so, some questions come
immediately to mind ...

(*) Do we have an example of a class which checks explicitly the
    sequence of methods that the client calls?  I do not want to
    introduce a second way of doing the same thing.  (To introduce the
    3rd or the seventeenth way is not such a big deal <grin />.)  If I
    need to invent a mechanism, I shall come back with questions.

(*) Do we have an example of doxygen comments about such checking?


Meanwhile, I am gonna to off into gdb and try to see repeated attempts
to free a statement handle.


Thanks,
Terry.