Hi Jim,
Have you added the corresponding code in request_tp_clear and
request_tp_traverse ?
I think you can check-in your code, since the debate about import
looks like it is going to take som time before the next release...
Regards,
Nicolas
2005/6/12, Jim Gallacher <[EMAIL PROTECTED]>:
> Nicolas Lehuen wrote:
> > Hi Jim,
> >
> > After a few checks (unittest + load testing), I've checked in my
> > modifications ; you might want to update and merge it with your code.
>
> I'm still getting a memory leak with the merged code. Should I commit my
> changes anyway, or maybe we could create new svn branch for this testing?
>
> Jim
>
> > Regards,
> > Nicolas
> >
> > 2005/6/12, Jim Gallacher <[EMAIL PROTECTED]>:
> >
> >>Nicolas Lehuen wrote:
> >>
> >>>You should cast self to a requestobject* :
> >>
> >>That worked. Running some tests.
> >>
> >>Jim
> >>
> >>
> >>>((requestobject*)self)->session
> >>>
> >>>Anyway, I'm currently rewriting the whole request_tp_traverse /
> >>>request_tp_clear / request_tp_clear functions like this :
> >>>
> >>>static void request_tp_dealloc(requestobject *self)
> >>>{
> >>> // de-register the object from the GC
> >>> // before its deallocation, to prevent the
> >>> // GC to run on a partially de-allocated object
> >>> PyObject_GC_UnTrack(self);
> >>>
> >>> request_tp_clear(self);
> >>>
> >>> PyObject_GC_Del(self);
> >>>}
> >>>
> >>>/**
> >>> ** request_tp_traverse
> >>> **
> >>> * Traversal of the request object
> >>> */
> >>>static int request_tp_traverse(requestobject* self, visitproc visit,
> >>>void *arg) {
> >>> int result;
> >>>
> >>> if(self->dict) {result = visit(self->dict,arg); if(result) return
> >>> result;}
> >>> if(self->connection) {result = visit(self->connection,arg);
> >>>if(result) return result;}
> >>> if(self->server) {result = visit(self->server,arg); if(result)
> >>>return result;}
> >>> if(self->next) {result = visit(self->next,arg); if(result) return
> >>> result;}
> >>> if(self->prev) {result = visit(self->prev,arg); if(result) return
> >>> result;}
> >>> if(self->main) {result = visit(self->main,arg); if(result) return
> >>> result;}
> >>> if(self->headers_in) {result = visit(self->headers_in,arg);
> >>>if(result) return result;}
> >>> if(self->headers_out) {result = visit(self->headers_out,arg);
> >>>if(result) return result;}
> >>> if(self->err_headers_out) {result =
> >>>visit(self->err_headers_out,arg); if(result) return result;}
> >>> if(self->subprocess_env) {result =
> >>>visit(self->subprocess_env,arg); if(result) return result;}
> >>> if(self->notes) {result = visit(self->notes,arg); if(result) return
> >>> result;}
> >>> if(self->phase) {result = visit(self->phase,arg); if(result) return
> >>> result;}
> >>>
> >>> // no need to Py_DECREF(dict) since the reference is borrowed
> >>> return 0;
> >>>}
> >>>
> >>>static int request_tp_clear(requestobject *self)
> >>>{
> >>> PyObject* tmp;
> >>> tmp=self->dict; self->dict=NULL; Py_XDECREF(tmp);
> >>> tmp=self->connection; self->connection=NULL; Py_XDECREF(tmp);
> >>> tmp=self->server; self->server=NULL; Py_XDECREF(tmp);
> >>> tmp=self->next; self->next=NULL; Py_XDECREF(tmp);
> >>> tmp=self->prev; self->prev=NULL; Py_XDECREF(tmp);
> >>> tmp=self->main; self->main=NULL; Py_XDECREF(tmp);
> >>> tmp=self->headers_in; self->headers_in=NULL; Py_XDECREF(tmp);
> >>> tmp=self->headers_out; self->headers_out=NULL; Py_XDECREF(tmp);
> >>> tmp=self->err_headers_out; self->err_headers_out=NULL; Py_XDECREF(tmp);
> >>> tmp=self->subprocess_env; self->subprocess_env=NULL; Py_XDECREF(tmp);
> >>> tmp=self->notes; self->notes=NULL; Py_XDECREF(tmp);
> >>> tmp=self->phase; self->phase=NULL; Py_XDECREF(tmp);
> >>> tmp=self->hlo; self->hlo=NULL; Py_XDECREF(tmp);
> >>> tmp=self->dict; self->dict=NULL; Py_XDECREF(tmp);
> >>> tmp=self->dict; self->dict=NULL; Py_XDECREF(tmp);
> >>> tmp=self->dict; self->dict=NULL; Py_XDECREF(tmp);
> >>> tmp=self->dict; self->dict=NULL; Py_XDECREF(tmp);
> >>>
> >>> return 0;
> >>>}
> >>>
> >>>As you can guess from the source code, we'll have to add some code for
> >>>the session member when you'll integrate your code in subversion.
> >>>
> >>>Regards,
> >>>Nicolas
> >>>
> >>>2005/6/12, Jim Gallacher <[EMAIL PROTECTED]>:
> >>>
> >>>
> >>>>Nicolas,
> >>>>
> >>>>It fails to compile when I add your bit of code. Sure looks like it
> >>>>should work though.
> >>>>
> >>>>requestobject.c: In function `request_tp_traverse':
> >>>>requestobject.c:1539: error: structure has no member named `session'
> >>>>requestobject.c:1540: error: structure has no member named `session'
> >>>>
> >>>>Just to be clear, it compiles and runs just fine without the extra check
> >>>> added into request_tp_traverse.
> >>>>
> >>>>Nicolas Lehuen wrote:
> >>>>
> >>>>
> >>>>>I'm re-reading the comment I wrote when I implemented tp_traverse :
> >>>>>
> >>>>>// only traverse its dictionary since other fields defined in
> >>>>>request_rec_mbrs with type T_OBJECT
> >>>>>// cannot be the source of memory leaks (unless you really want it)
> >>>>>
> >>>>>Turns out that we should at least traverse the next and prev in case
> >>>>>someone does something like req.next.my_prev = req. That's what I
> >>>>>meant by "unless you really want it".
> >>>>
> >>>>I wondered about that. :)
> >>>>
> >>>>Jim
> >>>>
> >>>>
> >>>>
> >>>>>It's pretty difficult to get
> >>>>>there (you need an internal redirect), but you can. So I'm going to
> >>>>>have a look at re-implementing the tp_traverse handler.
> >>>>>Regards,
> >>>>>Nicolas
> >>>>>
> >>>>>2005/6/12, Nicolas Lehuen <[EMAIL PROTECTED]>:
> >>>>>
> >>>>>
> >>>>>
> >>>>>>Duh, I get it. If you add a member to the request object, and this
> >>>>>>member is not referenced in the request object's dictionary, then you
> >>>>>>have to add a special case for it in the tp_traverse handler. In
> >>>>>>requestobject.c :
> >>>>>>
> >>>>>>/**
> >>>>>>** request_tp_traverse
> >>>>>>**
> >>>>>>* Traversal of the request object
> >>>>>>*/
> >>>>>>static int request_tp_traverse(PyObject *self, visitproc visit, void
> >>>>>>*arg) {
> >>>>>> PyObject *dict,*values,*item,*str;
> >>>>>> int i,size;
> >>>>>>
> >>>>>> // only traverse its dictionary since other fields defined in
> >>>>>>request_rec_mbrs with type T_OBJECT
> >>>>>> // cannot be the source of memory leaks (unless you really want it)
> >>>>>> dict=*_PyObject_GetDictPtr(self);
> >>>>>> if(dict) {
> >>>>>> // this check is not needed, I guess, _PyObject_GetDictPtr
> >>>>>>always give a pointer to a dict object.
> >>>>>> if(PyDict_Check(dict)) {
> >>>>>> i = visit(dict,arg);
> >>>>>> if(i) {
> >>>>>> ap_log_rerror(APLOG_MARK, APLOG_NOERRNO|APLOG_ERR, 0,
> >>>>>>((requestobject*)self)->request_rec, "%s:%i Call to visit()
> >>>>>>failed",__LINE__,__FILE__);
> >>>>>> // no need to Py_DECREF(dict) since the reference is
> >>>>>> borrowed
> >>>>>> return i;
> >>>>>> }
> >>>>>> }
> >>>>>> else {
> >>>>>> ap_log_rerror(APLOG_MARK, APLOG_NOERRNO|APLOG_ERR, 0,
> >>>>>>((requestobject*)self)->request_rec, "%s:%i Expected a
> >>>>>>dictionary",__LINE__,__FILE__);
> >>>>>> }
> >>>>>> }
> >>>>>> else {
> >>>>>> ap_log_rerror(APLOG_MARK, APLOG_NOERRNO|APLOG_ERR, 0,
> >>>>>>((requestobject*)self)->request_rec, "%s:%i Expected a
> >>>>>>dictionary",__LINE__,__FILE__);
> >>>>>> }
> >>>>>>
> >>>>>> /* This is the new code that needs to be added to support the new
> >>>>>>session member */
> >>>>>> if(self->session) {
> >>>>>> i = visit(self->session,arg);
> >>>>>> if(i) {
> >>>>>> ap_log_rerror(APLOG_MARK, APLOG_NOERRNO|APLOG_ERR, 0,
> >>>>>>((requestobject*)self)->request_rec, "%s:%i Call to visit()
> >>>>>>failed",__LINE__,__FILE__);
> >>>>>> return i;
> >>>>>> }
> >>>>>> }
> >>>>>>
> >>>>>> // no need to Py_DECREF(dict) since the reference is borrowed
> >>>>>> return 0;
> >>>>>>}
> >>>>>>
> >>>>>>Regards,
> >>>>>>Nicolas
> >>>>>>
> >>>>>>2005/6/12, Jim Gallacher <[EMAIL PROTECTED]>:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>>Hi Nicolas,
> >>>>>>>
> >>>>>>>Nicolas Lehuen wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>>Hi Jim,
> >>>>>>>>
> >>>>>>>>How where you creating the session object in requestobject.c ? If you
> >>>>>>>>were using PythonObject_New, then this may explain the memory leak.
> >>>>>>>>Objects that must be managed by the garbage collector have to be
> >>>>>>>>created with PyObject_GC_New.
> >>>>>>>
> >>>>>>>The c-code loads the python module and calls a function which generates
> >>>>>>>the session object.
> >>>>>>>
> >>>>>>>src/requestobject.c
> >>>>>>>static PyObject *req_get_session(requestobject *self, PyObject *args)
> >>>>>>>{
> >>>>>>> PyObject *m; // session module
> >>>>>>> PyObject *sid; // session id
> >>>>>>> PyObject *result;
> >>>>>>>
> >>>>>>> if (!self->session) {
> >>>>>>> sid = PyObject_CallMethod(self->subprocess_env, "get",
> >>>>>>> "(ss)","REDIRECT_PYSID", "");
> >>>>>>>
> >>>>>>> /********
> >>>>>>> * This is where the session instance is created
> >>>>>>> ********/
> >>>>>>> m = PyImport_ImportModule("mod_python.session2.TestSession");
> >>>>>>> self->session = PyObject_CallMethod(m, "create_session", "(OO)",
> >>>>>>> self, sid);
> >>>>>>>
> >>>>>>> Py_DECREF(m);
> >>>>>>> Py_DECREF(sid);
> >>>>>>> if (self->session == NULL)
> >>>>>>> return NULL;
> >>>>>>> }
> >>>>>>> result = self->session;
> >>>>>>> Py_INCREF(result);
> >>>>>>> return result;
> >>>>>>>}
> >>>>>>>
> >>>>>>>----------------------------------------------------------------
> >>>>>>>mod_python/session2/TestSession.py
> >>>>>>># emulate a simple test session
> >>>>>>>import _apache
> >>>>>>
> >>>>>>>from mod_python.Session import _new_sid
> >>>>>>
> >>>>>>
> >>>>>>>class TestSession(object):
> >>>>>>>
> >>>>>>> def __init__(self, req, sid=0, secret=None, timeout=0, lock=1):
> >>>>>>> req.log_error("TestSession.__init__")
> >>>>>>> # Circular Reference causes problem
> >>>>>>> self._req = req
> >>>>>>>
> >>>>>>> # keeping a reference to the server object does
> >>>>>>> # NOT cause a problem
> >>>>>>> self._server = req.server
> >>>>>>> self._lock = 1
> >>>>>>> self._locked = 0
> >>>>>>> self._sid = _new_sid(req)
> >>>>>>> self.lock()
> >>>>>>> self.unlock()
> >>>>>>>
> >>>>>>> def lock(self):
> >>>>>>> if self._lock:
> >>>>>>> _apache._global_lock(self._req.server, self._sid)
> >>>>>>> self._locked = 1
> >>>>>>>
> >>>>>>> def unlock(self):
> >>>>>>> # unlock will ocassionally segfault
> >>>>>>> if self._lock and self._locked:
> >>>>>>> _apache._global_unlock(self._req.server, self._sid)
> >>>>>>> self._locked = 0
> >>>>>>>
> >>>>>>>def create_session(req,sid):
> >>>>>>> return TestSession(req,sid)
> >>>>>>>
> >>>>>>
> >>
> >
>
>
