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. :) JimIt'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 _apachefrom mod_python.Session import _new_sidclass 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)