I have let myself write quite a bit in-line. Meanwhile, here are the actual questions arising.
(*) Do you agree that ODatabaseMetaDataResultSet ctor should warn if it does not get a statement handle? (*) Is there any value to a bug report at this stage? Thanks, Terry. On Wed, 2012-07-18 at 05:15 +0200, Lionel Elie Mamane wrote: > On Tue, Jul 17, 2012 at 03:46:14PM -0400, Terrence Enger wrote: > > 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 > > >> (...) 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? > > I'm not sure what you mean there. Something created a ODatabaseMetaDataResultSet. Presumably the client code had some intended purpose for the object. But the statement handle appeared in the ODBC log file only in SQLAllocHandle. I conclude that the client code did not do very much; as the client code did not even free the handle, there is no sign that the client made a deliberate decision to abandon its purpose. Hmm, that sounds like it could be a bug, or an opportunity for optimization, or an opportunity for simplification. I, too, am not sure exactly what I mean: the backtraces from construction of the otherwise-unused ODatabaseMetaDataResultSet objects each show scores of call levels. Even assuming that my hypothesized failed purpose is closer to the ctor than to main, I am daunted. > > >> 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.. > > Good point. However, calling disposing() multiple times does not lead > to double or triple freeing of the statement handle with the current > code. Oh my. I was wrong in so many ways ... > > ODatabaseMetaDataResultSet::disposing: > > ::osl::MutexGuard aGuard(m_aMutex); > if(m_bFreeHandle) > m_pConnection->freeStatementHandle(m_aStatementHandle); > > > So, m_pConnection->freeStatementHandle can be called multiple times, > OK. But look at OConnection::freeStatementHandle: > > void OConnection::freeStatementHandle(SQLHANDLE& _pHandle) > { > N3SQLFreeStmt(_pHandle,SQL_RESET_PARAMS); > N3SQLFreeStmt(_pHandle,SQL_UNBIND); > N3SQLFreeStmt(_pHandle,SQL_CLOSE); > N3SQLFreeHandle(SQL_HANDLE_STMT,_pHandle); > > _pHandle = SQL_NULL_HANDLE; > } > > 1) The argument is passed by reference > > 2) After freeing the handle, it sets the handle to a special dummy > value; because it is passed by reference, it is updated in the > caller, and DatabaseMetaDataResultSet::m_aStatementHandle is set to > the dummy value. > > 3) So the second time disposing() is called, SQLFreeHandle is called > on the dummy handle, not on the original "real" handle. > Exactly so. And moreover ... (*) I confused ODatabaseMetaDataResultSet::dispose, which the dtor calls, with ODatabaseMetaDataResultSet::disposing, which frees the statement handle. Sheesh, it's not as if the names differed in only one letter, or something. (*) It was manifest in gdb that the dtor does not call disposing. I was too close-minded to notice. (*) The ODBC log files do not show any null handles. Altogether, a bad day of work. Sorry. > The remaining doubt I could have is whether calling free on the NULL > handle is guaranteed safe; I don't see anything to that extent in the > ODBC docs, maybe it would be safer to add: > > if (_pHandle == SQL_NULL_HANDLE) > return; > > to the top of OConnection::freeStatementHandle? > Yes, I shall do that. It is more drastic what I was preparing to offer in this function. > > > 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 > > All these "exactly one" are too dangerous; as far as possible, the > class should be safe for "zero or more" of anything :) > > Except for things like "exactly one construct() before anything else", > which are pretty much guaranteed by C++ semantics :) > > > If the guess is plausible, should the class complain (SAL_WARN_IF, I > > presume) about violations of the protocol? > > If there are legitimate protocol limitations, it is good, at the very > least in debug mode, to check for protocol violations and react > accordingly; from a warning to an exception, as appropriate. > > My first guess is that there are no such legitimate limitations here, > but you've looked at this code more than me; if you think there are, > let me know. > > The removal of m_bCloseHandle makes it much easier to understand the code, so I no longer feel the need to invent elaborate hypotheses. Add your skepticism to this, and I think the question is dead. (I may hack the class to warn of an object essentially unused throughout its lifetime, but that is a bit of ad-hocery that should not be inflicted on anybody else.) I am making one more gratuitous change: ODatabaseMetaDataResultSet ctor warns if it does not get a statment handle. The justification is ... well, I guess it's just my nast, suspicious mind. May I have your blessing? Thank you for your patience with me. Terry. _______________________________________________ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice