Sebastien,
fyi I'm looking and this and will have a different "fix" up there later
today/tomorrow so no need for us both to look at it ;-)
Cheers,
On 10/10/06, Pete Robbins <[EMAIL PROTECTED]> wrote:
On 10/10/06, Jean-Sebastien Delfino <[EMAIL PROTECTED]> wrote:
>
> Pete Robbins wrote:
> > We need to consider the pass by-value vs pass by-reference semantics,
> > including "deep copying" the DataObject tree. I have thought about his
>
> > and
> > we should discuss this after M2. I'll put together a proposal on
> another
> > thread.
> >
> > For now though I would change your patch and have Operation allocate
> and
> > free DataObjectPtrs as appropriate. The memory leak of the odd int,
> > double,
> > or even a string is not going to be significant but with DataObjectPtr
> > the
> > DOs will never get freed... or the DataFactory that created them
> ...etc..
> >
> >
> > void Operation::addParameter(const DataObjectPtr *parm)
> > {
> > LOGINFO(4, "Operation::addParameter(DataObjectPtr)");
> > parameters.insert(parameters.end(), Parameter((void*)parm,
> > DATAOBJECT));
> > }
> >
> > would become
> >
> > void Operation::addParameter(const DataObjectPtr *parm)
> > {
> > LOGINFO(4, "Operation::addParameter(DataObjectPtr)");
> > parameters.insert(parameters.end(), Parameter((void*)new
> > DataObjectPtr(*parm), DATAOBJECT));
> > }
> >
> > and the Operation destructor would need to find all the DATAOBJECT
> type
> > paramters and delete the DataObjectPtr.
> >
> > Cheers,
> > On 10/10/06, Jean-Sebastien Delfino <[EMAIL PROTECTED]> wrote:
> >>
> >> Pete Robbins wrote:
> >> > On 10/10/06, Jean-Sebastien Delfino < [EMAIL PROTECTED]> wrote:
> >> >>
> >> >> On the plane on my way to ApacheCon I was playing around with our
> Web
> >> >> Services support (adding a Web Services client to BigBank) and
> >> found a
> >> >> serious problem in Axis2Client and WSServiceProxy, which allocate
> the
> >> >> DataObject pointers they return on the stack instead of doing a
> new
> >> >> DataObjectPtr. This causes a memory violation if a client tries to
>
> >> >> access a DataObject returned from a Web service.
> >> >>
> >> >> I raised JIRA TUSCANY-816 for this, have the fix but need to do a
> >> little
> >> >> more testing before I commit it. If things go well I should be
> >> able to
> >> >> commit it later this evening.
> >> >
> >> >
> >> > Having a quick glance at this code I was wondering who frees up the
> >> > memory
> >> > allocated by Axis2Client and set in the Operation?
> >>
> >> Currently, nobody :) We had already started to discuss this issue in
> >> this thread:
> >>
> >>
> http://mail-archives.apache.org/mod_mbox/ws-tuscany-dev/200609.mbox/[EMAIL
PROTECTED]
> >>
> >> but we don't have a definitive solution for it yet.
> >>
> >> > For DataObjectPtr we could have the
> >> > Operation::setParameter/setReturnValue
> >> > methods new up a DataObjectPtr and free it in the destructor. This
> >> would
> >> > work nicely with this as it is reference counted. A different
> solution
> >> > would
> >> > be needed for the other types.
> >>
> >> I think you're right, but we need to take a close look at the
> lifecycle
> >> of the data flowing through the runtime. For example, data to a
> client
> >> component returned from a service invocation cannot be freed until
> the
> >> client component finishes.
> >>
> >> Here's a first analysis of how we could handle the data set into an
> >> Operation parameters or returnValue. It may not be completely right
> but
> >> it's a starting point to help trigger some ideas...
> >>
> >> Parameters:
> >> - a DataObjectPtr set into the parameter --> can be freed when the
> >> Operation is deleted (in the ServiceProxy::invoke method just before
> >> returning)
> >> - the DataObject pointed to by that DataObjectPtr --> will be freed
> when
> >> its reference count goes down to 0
> >> - a string* or int* or char** set into the parameter --> can be freed
>
> >> when the Operation is deleted
> >> - the string or int pointed to by that string* or int* --> can be
> freed
> >> when the Operation is deleted
> >> - the char* --> document that the component getting the char* is
> >> responsible for freeing it? or free it when the Operation is deleted?
> >>
> >> Return value:
> >> - a DataObjectPtr set into the return value --> can be freed when
> the
> >> Operation is deleted
> >> - the DataObject pointed to by that DataObjectPtr --> will be freed
> when
> >> its reference count goes down to 0
> >> - a string* or int* or char** set into the parameter --> can be freed
> >> when the Operation is deleted
> >> - the string or int pointed to by that string* or int* --> can be
> freed
> >> when the Operation is deleted
> >> - the char* --> document that the component getting the char* is
> >> responsible for freeing it? or?
> >>
> >> Anyway, I'm not sure what you guys think, but unless we have a very
> >> clear view of how to fix this, I'd suggest to tackle this problem
> after
> >> the M2 release and live with the small memory leak for now.
> >>
> >> Thoughts?
> >>
> >> >
> >> > Cheers,
> >> >
> >> >
> >>
> >> --
> >> Jean-Sebastien
> >>
> >>
> >> ---------------------------------------------------------------------
> >> To unsubscribe, e-mail: [EMAIL PROTECTED]
> >> For additional commands, e-mail: [EMAIL PROTECTED]
> >>
> >>
> >
> >
>
> Ah OK I didn't realize that not freeing the DataObjectPtr was going to
> keep all the related DataObjects around... What you're proposing looks
> good to me. For M2 I agree with you that we should only handle the
> DataObject case, but I'm curious: will the same technique apply to the
> other types as well then?
Not quite. DataObjectPtr is easy as it is reference counted. For the
others we can have Operation allocate the memory and free it for cases when
we are passing by value. When we are passing by reference then Operation can
not allocate/delete the memory as it is owned by the "client". I think this
isn't too hard to fix by adding a flag to Operation::setParameter to say if
we are passing by value (default) or reference. If by reference then
Operation does nothing, if by value it allocates/deletes the parameter
memory.
The additional complication is handling DataObjectPtr in pass by value.
Here we should really take a deep copy of the DataObject. Again this is
fairly simple to do.
And the final(?) complication is pass by value for "other types". char* we
can strcpy and manage in Operation. std::string we can also copy. The
problem comes if a user wants to pass a MyClass on an operation. The runtime
does not know how to copy this. We are fine passing this by reference. So
this is currently a restriction on the C++ interfaces allowed in pass by
value. This is quite tricky to fix.
Cheers,
--
> Pete
--
Pete
