<This message was supposed to be sent to the whole list, but I
accidently sent it only to Fabrizio. So sorry Fabrizio for getting
this twice.>
Hi Fabrizio,
I see that I did miss the case where the message is placed into the
queue. However, my comments about using a smart pointer (although
you'd need a shared smart pointer instead of an auto_ptr) still
remains. I did a little more poking around the code and found
XrdClientConn::ReadPartialAnswer(...)
...
Xmsg = ConnectionManager->ReadMsg(fLogConnID);
...
if (HasToAlloc) {
tmp2MoreData = realloc(*tmpMoreData, TotalBlkSize +
Xmsg->DataLen());
if (!tmp2MoreData) {
Error("ReadPartialAnswer", "Error reallocating " <<
TotalBlkSize << " bytes.");
free(*tmpMoreData);
*tmpMoreData = 0;
what_to_do = kTSRHReturnNullMex;
return 0;
}
So here Xmsg (which normally is returned from this routine if
everything was OK) is obtained from a method that does not put the
message on a queue. So if we have this allocation problem, the message
is not passed up the chain nor is it deleted. I understand that this
is an unlikely condition, but using smart pointers to manage the memory
would take care of the standard and the unusual conditions. [Note: I
found this after looking at the code for less than 10 minutes and have
spent no further time. Therefore I do not know if this is the only
leak or if there are others.]
Resource management is tough in C++ and using smart pointers makes the
compiler do the work for you (which I like since I'm lazy).
Chris
On Oct 13, 2004, at 3:33 AM, Fabrizio Furano wrote:
> Hi Chris,
>
> I know them all, and will put my eye on them in a short time. Anyway
> it's alpha code and I am revisiting it all in these days.
>
> BTW, this remark:
>
> > Code:
> > XrdClientPhyConnection::BuildMessage returns a new
> XrdClientMessage,
> > but in SocketReaderThread we see
> >
> > while (1) {
> > thisObj->BuildMessage(TRUE, TRUE);
> > thisObj->CheckAutoTerm();
> > }
> > the return value is ignored
> >
> > Resolution:
> > -delete the value returned from the line '
> > thisObj->BuildMessage(TRUE, TRUE);'
> > -better solution is to have XrdClientPhyConnection::BuildMessage
> > return a
> > std::auto_ptr<XrdClientMessage>
>
> is not correct, since the BuildMessage parameters tell it to enqueue
> what it created. So it's not a good idea to delete a message that has
> been enqueued somewhere else.
> That leak is related to a condition for the message deletion inside
> SendGenCommand/ClientServerCmd. It does not loose all the messages.
> Only one in a particular situation. This does not mean that it won't
> be fixed.
>
> Fabrizio
