[Json-rpc-java] Clientside memory issues with the JSON RPC JavaScript
Eric Pascarello
alienfrog at gmail.com
Tue Oct 30 05:34:36 SGT 2007
1) When I do my talks on Ajax, I always say to stay away from synch calls,
but your new code still allows for synch. requests. Therefore the issue
still remains. It needs the delete statement in the !callback check unless
you are truely going to stop people from making those calls.
2) I am not seeing the circular reference since it looks like you guys redid
that portion.
Other improvements looking over the code?
*JSONRpcClient.async_inflight
One thing that still has me scratching my head is why is
JSONRpcClient.async_inflight storing the request object when it is only used
as a Boolean? Seems like a waste of memory to see if the request was
cancellled. Only reason why i would see holding the object is if you were
going to implement some sort of retry code in this. I would change it to an
array holding a Boolean if you are not going to use it for anything else.
*fetching the Native/ActiveX
I think there should also be object detection around the checks for native
and for activeX.
if(window.XMLHttpRequest){
try...catch
}
if(window.ActiveXObject){
for
try.. catch
}
It keeps the browser from wasting some time to make calls. You can also use
something like this
http://ajaxian.com/archives/lazy-function-definition-pattern so you do not
have to do a look up every single time we need a new XHR object. We only
need to look at it once, no need to search for the correct version every
single time.
*onreadystatechange
Take a look at a library like YUI connection manager and see how they handle
it. They using polling instead because of memory leak issues.
*http.abort()
Personal preference, if I am going to reuse the object, I run abort which
resets the repsonse and other properties back to their default states. I
would do it when it is returned to the pool.
Final thing is look at sIEve [http://sourceforge.net/projects/ieleak/] when
developing on Windows and watch out for IE6.
Regards,
Eric Pascarello
On 10/29/07, Arthur Blake <arthur.blake at gmail.com > wrote:
>
> Hi Eric.
> Just a few quick notes.
>
> 1. Synchronous calls have been deprecated and we don't recommend using
> them at all anymore. See http://code.google.com/p/jabsorb/issues/detail?id=25
>
>
> 2. We've done quite a bit of refactoring of jsonrpc.js. If you don't mind
> examining the latest version to see if you see these same issues, we'd
> appreciate it. I will do the same when I have time. If the issues still
> exist, we'll create bug tracker entries for them and address them as soon as
> possible.
> (see http://jabsorb.googlecode.com/svn/trunk/webapps/jsonrpc/jsonrpc.js for
> latest version of the code.)
>
> 3. I agree that jsonrpc.js is a mess, and I think it needs a total
> overhaul... we'll be working on that in upcoming jabsorb releases.
>
> See http://code.google.com/p/jabsorb/ for the latest release of jabsorb,
> (formerly known as json-rpc-java.)
>
> On 10/29/07, Eric Pascarello < alienfrog at gmail.com> wrote:
>
> > Hello everyone,
> >
> > I have been working on memory issues with an application and I found
> > some
> > major issues within the core javascript of the JSON RPC. There are two
> > bugs
> > I have found so far and there are other improvements that can take
> > place.
> > With this thread I will only talk about the memory issues.
> >
> > The bugs:
> > 1) The object JSONRpcClient.async_inflight is growing with sychronous
> > requests.
> > 2) There is a circular reference within all of the Ajax calls.
> >
> > For the JSONRpcClient.async_inflight, who ever coded it forgot to remove
> > the
> > current request from the object for synchronous requests. The code
> > properly
> > removes itt for asychronous requests. You can see that by looking at the
> >
> > code within the onreadystatechnage callback.
> >
> > To test for this bug, fire off a series of Synch requests and with
> > Firebug
> > console [ http://www.getFirebug.com], type JSONRpcClient.async_inflightinto
> > the command line. Doing this will allow you to see what is in the array
> > at
> > that given time.
> >
> > So I changed
> >
> > if(!req.cb)
> > return this._handleResponse(http);
> >
> > to this
> >
> > if(!req.cb){
> > delete JSONRpcClient.async_inflight[req.requestId];
> > return this._handleResponse(http);
> > }
> >
> > and JSONRpcClient.async_inflight is no longer gaining in size. That was
> > an
> > easy fix. :)
> >
> >
> > The next issue was a circular reference. If you were to inspect the
> > code,
> > and look at the DOM tree, you would see:
> >
> >
> > - System
> > - listMethods
> > - client
> > - System
> > - listMethods
> > - client
> > - To Infinity and beyond
> >
> > Firebug is a great thing because it lets you inspeact the objects. To
> > test
> > for this, you can add the line console.log(this); inside of the
> > JSONRpcClient constructor I am [thinking it somewhere around 115-118.]
> > Click
> > on the Object in the console tab and you can navigate the tree and see
> > the
> > circular reference first hand.
> >
> > The circular reference problem is in the following chunk of code in
> > _createMethod():
> >
> > JSONRpcClient.prototype._createMethod =
> > function JSONRpcClient_createMethod(methodName)
> > {
> > var fn=function()
> > {
> > var args = [];
> > var callback = null;
> > for(var i=0;i< arguments.length;i++) args.push(arguments[i]);
> > if(typeof args[0] == "function") callback = args.shift();
> > var req = fn.client._makeRequest.call(fn.client, fn.methodName,
> > args, callback);
> > if(callback == null) {
> > return fn.client._sendRequest.call(fn.client, req);
> > } else {
> > JSONRpcClient.async_requests.push(req);
> > JSONRpcClient.kick_async();
> > return req.requestId ;
> > }
> > };
> > fn.client = this;
> > fn.methodName = methodName;
> > return fn;
> > };
> >
> > The whole problem relates to this line fn.client = this;. They than use
> > fn.client inside the function to be able to reference the make and send
> > methods. Storing this is in an another object is usually a very
> > bad thing
> > to do. What should have been done is used a local variable and reference
> > that. This keeps us from causing the endless loop of it refering to
> > itself
> > over and over again. Something like this would do the trick:
> >
> > JSONRpcClient.prototype._createMethod =
> > function JSONRpcClient_createMethod(methodName)
> > {
> >
> > var ref = this;
> >
> > var fn=function()
> > {
> > var args = [];
> > var callback = null;
> > for(var i=0;i<arguments.length;i++) args.push(arguments[i]);
> > if(typeof args[0] == "function") callback = args.shift();
> > var req = ref._makeRequest.call(ref, fn.methodName,
> > args, callback);
> > if(callback == null) {
> > return ref._sendRequest.call(ref, req);
> > } else {
> > JSONRpcClient.async_requests.push(req);
> > JSONRpcClient.kick_async ();
> > return req.requestId;
> > }
> > };
> > fn.methodName = methodName;
> > return fn;
> > };
> >
> > Now there is no circular reference when you examine it in the Firebug
> > console. I used var ref = this; to allow me to reference this. We no
> > longer
> > have to reference client in this chunk of code.
> >
> > Hopefully someone out there will update the project with the fixes so
> > they
> > can have some of their memory back. In the future we can talk about
> > improving the code in some other areas.
> >
> > Regards,
> > Eric Pascarello
> > _______________________________________________
> > Json-rpc-java mailing list
> > Json-rpc-java at oss.metaparadigm.com
> > http://oss.metaparadigm.com/mailman/listinfo/json-rpc-java
> >
>
>
More information about the Json-rpc-java
mailing list