Re: svn commit: r757671 - in /ofbiz/trunk: ./ framework/common/servicedef/ framework/example/testdef/assertdata/ framework/service/src/org/ofbiz/service/ framework/service/src/org/ofbiz/service/jms/ framework/webapp/src/org/ofbiz/webapp/control/ framework/...

[Adam Heath-2]

[hidden email] wrote:

> Author: jonesde
> Date: Tue Mar 24 06:27:03 2009
> New Revision: 757671
>
> URL: http://svn.apache.org/viewvc?rev=757671&view=rev
> Log:
> Removed some more references to applications stuff from framework, including small cleanup of service engine to get rid of WebAppDispatcher and use new factory method in GenericDispatcher, and changed AvailableServices.groovy to get the real dispatcher list instead of a hard-coded one; also updated eclipse classpath file for new webslinger stuff
> Modified: ofbiz/trunk/framework/webslinger/src/org/ofbiz/webslinger/WebslingerContextMapper.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/webslinger/src/org/ofbiz/webslinger/WebslingerContextMapper.java?rev=757671&r1=757670&r2=757671&view=diff
> ==============================================================================
> --- ofbiz/trunk/framework/webslinger/src/org/ofbiz/webslinger/WebslingerContextMapper.java (original)
> +++ ofbiz/trunk/framework/webslinger/src/org/ofbiz/webslinger/WebslingerContextMapper.java Tue Mar 24 06:27:03 2009
> @@ -18,56 +18,33 @@
>   *******************************************************************************/
>  package org.ofbiz.webslinger;
>  
> -import java.io.File;
>  import java.io.IOException;
> -import java.lang.ref.SoftReference;
>  import java.net.URL;
>  import java.util.ArrayList;
>  import java.util.Arrays;
> -import java.util.HashMap;
>  import java.util.HashSet;
> -import java.util.Iterator;
>  import java.util.List;
>  import java.util.Set;
>  
> -import javax.management.JMException;
> -import javax.naming.NamingException;
>  import javax.servlet.ServletConfig;
>  import javax.servlet.ServletContext;
>  import javax.servlet.ServletException;
>  import javax.servlet.http.HttpServletRequest;
> -import javax.servlet.http.HttpServletResponse;
> -import javax.xml.parsers.ParserConfigurationException;
>  
> -import org.xml.sax.SAXException;
> -
> -import org.apache.commons.vfs.FileObject;
> -import org.apache.commons.vfs.FileSystemException;
> -import org.apache.commons.vfs.FileSystemManager;
> -import org.apache.commons.vfs.NameScope;
> -
> -import org.ofbiz.base.util.UtilFormatOut;
>  import org.ofbiz.base.util.UtilMisc;
>  import org.ofbiz.base.util.UtilProperties;
> -import org.ofbiz.base.util.UtilValidate;
> -import org.ofbiz.entity.GenericEntity;
> -import org.ofbiz.entity.GenericEntityException;
>  import org.ofbiz.entity.GenericDelegator;
> +import org.ofbiz.entity.GenericEntityException;
>  import org.ofbiz.entity.GenericValue;
> -import org.ofbiz.entity.transaction.TransactionFactory;
>  import org.ofbiz.entity.cache.Cache;
>  import org.ofbiz.entity.util.EntityUtil;
>  import org.ofbiz.security.SecurityFactory;
> +import org.ofbiz.service.GenericDispatcher;
>  import org.ofbiz.service.LocalDispatcher;
> -import org.ofbiz.service.WebAppDispatcher;
> -
>  import org.webslinger.AbstractMappingWebslingerServletContextFactory;
>  import org.webslinger.WebslingerServletContext;
> -import org.webslinger.container.WebslingerContainer;
> -import org.webslinger.lang.ObjectUtil;
> -import org.webslinger.servlet.WebslingerServlet;
> -import org.webslinger.util.TTLObject;
>  import org.webslinger.collections.CollectionUtil;
> +import org.webslinger.lang.ObjectUtil;
>  
>  public class WebslingerContextMapper extends AbstractMappingWebslingerServletContextFactory {
>      protected ServletContext servletContext;
> @@ -132,7 +109,7 @@
>              }
>          }
>          System.err.println(readerURLs);
> -        return new WebAppDispatcher(name, delegator, readerURLs);
> +        return GenericDispatcher.getLocalDispatcher(name, delegator, readerURLs, null);
>      }
>  
>      protected Set<String> getSuffixes() throws Exception {
>

There maybe side-effects with this.  Webslinger is a nested servlet
container.  It runs inside any normal, plain-jane servlet container,
then 'fakes out' other servlets, so it actually looks like they are in
the webslinger servlet container.  It does it's own path matching,
hostname matching, real servlet path lookup, web.xml parsing, etc.

The line you modified is running inside this nested, fake servlet
space.  The name used in this code path, is the file location of the
mapped website; this is *not* the webapps that exist in
framework/*/webapp, but the content management side of things.  The
embedded readerURLs are fetched from the virtualized web.xml.

Due to the nature of COW and other side effects, this change breaks
the webslinger contract, of *not* storing stuff in static space that
can't be overridden by either reconstructing the object, or just
reloading(using some kind of ttl/timestamp based cache).

[David E Jones-3]

On Mar 25, 2009, at 11:37 AM, Adam Heath wrote:

> [hidden email] wrote:
>> Author: jonesde
>> Date: Tue Mar 24 06:27:03 2009
>> New Revision: 757671
>>
>> URL: http://svn.apache.org/viewvc?rev=757671&view=rev
>> Log:
>> Removed some more references to applications stuff from framework,  
>> including small cleanup of service engine to get rid of  
>> WebAppDispatcher and use new factory method in GenericDispatcher,  
>> and changed AvailableServices.groovy to get the real dispatcher  
>> list instead of a hard-coded one; also updated eclipse classpath  
>> file for new webslinger stuff
>> Modified: ofbiz/trunk/framework/webslinger/src/org/ofbiz/webslinger/
>> WebslingerContextMapper.java
>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/webslinger/src/org/ofbiz/webslinger/WebslingerContextMapper.java?rev=757671&r1=757670&r2=757671&view=diff
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =====================================================================
>> --- ofbiz/trunk/framework/webslinger/src/org/ofbiz/webslinger/
>> WebslingerContextMapper.java (original)
>> +++ ofbiz/trunk/framework/webslinger/src/org/ofbiz/webslinger/
>> WebslingerContextMapper.java Tue Mar 24 06:27:03 2009
>> @@ -18,56 +18,33 @@
>>  
>> *******************************************************************************/
>> package org.ofbiz.webslinger;
>>
>> -import java.io.File;
>> import java.io.IOException;
>> -import java.lang.ref.SoftReference;
>> import java.net.URL;
>> import java.util.ArrayList;
>> import java.util.Arrays;
>> -import java.util.HashMap;
>> import java.util.HashSet;
>> -import java.util.Iterator;
>> import java.util.List;
>> import java.util.Set;
>>
>> -import javax.management.JMException;
>> -import javax.naming.NamingException;
>> import javax.servlet.ServletConfig;
>> import javax.servlet.ServletContext;
>> import javax.servlet.ServletException;
>> import javax.servlet.http.HttpServletRequest;
>> -import javax.servlet.http.HttpServletResponse;
>> -import javax.xml.parsers.ParserConfigurationException;
>>
>> -import org.xml.sax.SAXException;
>> -
>> -import org.apache.commons.vfs.FileObject;
>> -import org.apache.commons.vfs.FileSystemException;
>> -import org.apache.commons.vfs.FileSystemManager;
>> -import org.apache.commons.vfs.NameScope;
>> -
>> -import org.ofbiz.base.util.UtilFormatOut;
>> import org.ofbiz.base.util.UtilMisc;
>> import org.ofbiz.base.util.UtilProperties;
>> -import org.ofbiz.base.util.UtilValidate;
>> -import org.ofbiz.entity.GenericEntity;
>> -import org.ofbiz.entity.GenericEntityException;
>> import org.ofbiz.entity.GenericDelegator;
>> +import org.ofbiz.entity.GenericEntityException;
>> import org.ofbiz.entity.GenericValue;
>> -import org.ofbiz.entity.transaction.TransactionFactory;
>> import org.ofbiz.entity.cache.Cache;
>> import org.ofbiz.entity.util.EntityUtil;
>> import org.ofbiz.security.SecurityFactory;
>> +import org.ofbiz.service.GenericDispatcher;
>> import org.ofbiz.service.LocalDispatcher;
>> -import org.ofbiz.service.WebAppDispatcher;
>> -
>> import org.webslinger.AbstractMappingWebslingerServletContextFactory;
>> import org.webslinger.WebslingerServletContext;
>> -import org.webslinger.container.WebslingerContainer;
>> -import org.webslinger.lang.ObjectUtil;
>> -import org.webslinger.servlet.WebslingerServlet;
>> -import org.webslinger.util.TTLObject;
>> import org.webslinger.collections.CollectionUtil;
>> +import org.webslinger.lang.ObjectUtil;
>>
>> public class WebslingerContextMapper extends  
>> AbstractMappingWebslingerServletContextFactory {
>>     protected ServletContext servletContext;
>> @@ -132,7 +109,7 @@
>>             }
>>         }
>>         System.err.println(readerURLs);
>> -        return new WebAppDispatcher(name, delegator, readerURLs);
>> +        return GenericDispatcher.getLocalDispatcher(name,  
>> delegator, readerURLs, null);
>>     }
>>
>>     protected Set<String> getSuffixes() throws Exception {
>>
>
> There maybe side-effects with this.  Webslinger is a nested servlet
> container.  It runs inside any normal, plain-jane servlet container,
> then 'fakes out' other servlets, so it actually looks like they are in
> the webslinger servlet container.  It does it's own path matching,
> hostname matching, real servlet path lookup, web.xml parsing, etc.
>
> The line you modified is running inside this nested, fake servlet
> space.  The name used in this code path, is the file location of the
> mapped website; this is *not* the webapps that exist in
> framework/*/webapp, but the content management side of things.  The
> embedded readerURLs are fetched from the virtualized web.xml.
>
> Due to the nature of COW and other side effects, this change breaks
> the webslinger contract, of *not* storing stuff in static space that
> can't be overridden by either reconstructing the object, or just
> reloading(using some kind of ttl/timestamp based cache).

Well, I certainly wouldn't want to violate any contracts... Heck, I'm  
not even sure if I have the authority to authorize violation of this  
contract.

This can be changed back to calling the constructor instead of the  
factory method. There is now a constructor on GenericDispatcher that  
mimics what the old WebAppDispatcher constructor did (WebAppDispatcher  
was kind of a bad name since all it did was allow you to specify  
additional reader URLs, which technically has nothing to do with  
webapps).

-David

[Adam Heath-2]

Adam Heath wrote:
> [hidden email] wrote:
>> Author: jonesde
>> Date: Tue Mar 24 06:27:03 2009
>> New Revision: 757671

I've had a chance to look over this change, the removal of
WebAppDispatcher.  And, it could break some code.

WebAppDispatcher has a check for a null classloader; if so, it uses
the Thread's classloader.

However, the standard GenericDispatcher does *not* do this; instead,
it uses the classloader of GenericDispatcher.class.

So, I consider this change to actually introduce a *regression*, even
if it doesn't actually cause any bugs in ofbiz trunk.

Additionally, ofbiz maintains a global cache of dispatchers by name.
This is a big nono for webslinger.  Webslinger has a *single* lookup
point, and then everything else it uses is keyed off of that.  This
allows for recycling to be done, by just removing the top-level reference.

[Jacques Le Roux]

Please Adam,

It would be cool if you could copy at least some lines of the previous msg in the thread. This is not specific to this msg of course
;o)

TIA

Jacques

From: "Adam Heath" <[hidden email]>

> Adam Heath wrote:
>> [hidden email] wrote:
>>> Author: jonesde
>>> Date: Tue Mar 24 06:27:03 2009
>>> New Revision: 757671
>
> I've had a chance to look over this change, the removal of
> WebAppDispatcher.  And, it could break some code.
>
> WebAppDispatcher has a check for a null classloader; if so, it uses
> the Thread's classloader.
>
> However, the standard GenericDispatcher does *not* do this; instead,
> it uses the classloader of GenericDispatcher.class.
>
> So, I consider this change to actually introduce a *regression*, even
> if it doesn't actually cause any bugs in ofbiz trunk.
>
> Additionally, ofbiz maintains a global cache of dispatchers by name.
> This is a big nono for webslinger.  Webslinger has a *single* lookup
> point, and then everything else it uses is keyed off of that.  This
> allows for recycling to be done, by just removing the top-level reference.
>