Return-Path: Mailing-List: contact python-dev-help@httpd.apache.org; run by ezmlm Delivered-To: mailing list python-dev@httpd.apache.org Received: (qmail 9695 invoked by uid 99); 12 Jun 2005 20:54:00 -0000 X-ASF-Spam-Status: No, hits=-0.0 required=10.0 tests=SPF_PASS X-Spam-Check-By: apache.org Received-SPF: pass (hermes.apache.org: domain of jg.lists@sympatico.ca designates 209.226.175.74 as permitted sender) Received: from tomts20.bellnexxia.net (HELO tomts20-srv.bellnexxia.net) (209.226.175.74) by apache.org (qpsmtpd/0.28) with ESMTP; Sun, 12 Jun 2005 13:53:59 -0700 Received: from [192.168.1.2] ([70.49.8.144]) by tomts20-srv.bellnexxia.net (InterMail vM.5.01.06.10 201-253-122-130-110-20040306) with ESMTP id <20050612205356.PTNQ19894.tomts20-srv.bellnexxia.net@[192.168.1.2]>; Sun, 12 Jun 2005 16:53:56 -0400 Message-ID: <42ACA1D7.8000303@sympatico.ca> Date: Sun, 12 Jun 2005 16:57:59 -0400 From: Jim Gallacher User-Agent: Mozilla Thunderbird 1.0 (X11/20041206) X-Accept-Language: en-us, en MIME-Version: 1.0 To: nicolas@lehuen.com CC: python-dev@httpd.apache.org Subject: Re: session handling - the next generation References: <42A622D4.2040406@sympatico.ca> <42AC44F0.9030206@sympatico.ca> <42AC508F.30108@sympatico.ca> <42AC72B9.4040103@sympatico.ca> <42AC7EFD.5090909@sympatico.ca> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-Virus-Checked: Checked 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 : > >>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 : >>> >>> >>>>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 : >>>>> >>>>> >>>>> >>>>>>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 : >>>>>> >>>>>> >>>>>> >>>>>>>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) >>>>>>> >>>>>> >> >