Re: [xwiki-devs] [xwiki-notifications] r15096 - in sandbox/xwiki-core-rest/src/main: java/components/org/xwiki/rest resources/META-INF/plexus
Hi Fabio, I can't see the reply you got on the list. Also I think you've introduced an important bug: whenever plexus instantiates a per-lookup component a new instance is created but you have to release it explicitely as otherwise it won't be released. Which means you'll get an out of memory exception very quickly. per-look must be avoided as much as possible (we don't have any in the other components of xwiki because of the release problem). Also I don't understand why something like XmlTextSpacesRepresenter should not be a singleton. Thanks -Vincent On Jan 6, 2009, at 4:43 PM, fmancinelli (SVN) wrote:
Author: fmancinelli Date: 2009-01-06 16:43:28 +0100 (Tue, 06 Jan 2009) New Revision: 15096
Added: sandbox/xwiki-core-rest/src/main/java/components/org/xwiki/rest/ XWikiRouter.java Removed: sandbox/xwiki-core-rest/src/main/java/components/org/xwiki/rest/ XWikiResourceFinder.java Modified: sandbox/xwiki-core-rest/src/main/java/components/org/xwiki/rest/ XWikiResource.java sandbox/xwiki-core-rest/src/main/java/components/org/xwiki/rest/ XWikiRestApplication.java sandbox/xwiki-core-rest/src/main/resources/META-INF/plexus/ components.xml Log: Refactoring due to the reply on the Restlet forum concerning object lifecycle (http://restlet.tigris.org/ds/viewMessage.do?dsForumId=4447&dsMessageId=10073... )
Now resources and presenters are instantiated at every lookup.
Code simplification.
Modified: sandbox/xwiki-core-rest/src/main/java/components/org/xwiki/ rest/XWikiResource.java =================================================================== --- sandbox/xwiki-core-rest/src/main/java/components/org/xwiki/rest/ XWikiResource.java 2009-01-06 14:29:26 UTC (rev 15095) +++ sandbox/xwiki-core-rest/src/main/java/components/org/xwiki/rest/ XWikiResource.java 2009-01-06 15:43:28 UTC (rev 15096) @@ -18,25 +18,20 @@ */ public class XWikiResource extends WadlResource { - /* This is injected by the component manager and containsall the registered representers */ + /* This is injected by the component manager and contains all the registered representers */ private Map<String, XWikiResourceRepresenter> descriptorToRepresenterMap;
/* This map contains only the the representers for the current resource, indexed by media type */ private Map<MediaType, XWikiResourceRepresenter> mediaTypeToRepresenterMap;
+ /* This is configured in components.xml */ + private String uriPattern; + @Override public void init(Context context, Request request, Response response) { super.init(context, request, response);
- /* - * If the mediaTypeToRepresenterMap is already initialized then return because we already have all the - * representers registered. - */ - if (mediaTypeToRepresenterMap != null) { - return; - } - mediaTypeToRepresenterMap = new HashMap<MediaType, XWikiResourceRepresenter>(); getVariants().clear(); for (String hint : descriptorToRepresenterMap.keySet()) { @@ -45,7 +40,7 @@ if (hint.startsWith(this.getClass().getName())) { getLogger().log( Level.INFO, - String.format("Registering %s representer for resource %s", representer.getMediaType(), this + String.format("Registering '%s' representer for resource %s", representer.getMediaType(), this .getClass().getName())); getVariants().add(new Variant(representer.getMediaType()));
mediaTypeToRepresenterMap.put(representer.getMediaType(), representer); @@ -60,9 +55,21 @@ if (representer != null) { return representer; } else { - /* Flow control should never reach this. */ + /* Flow control should never reach this point. */ + getLogger() + .log( + Level.WARNING, + String + .format( + "'%s' representer for resource %s doesn't exist. This should never happen. Returning a null representer.", + variant.getMediaType(), this.getClass().getName())); return new NullRepresenter(); } }
+ public String getUriPattern() + { + return uriPattern; + } + }
Deleted: sandbox/xwiki-core-rest/src/main/java/components/org/xwiki/ rest/XWikiResourceFinder.java =================================================================== --- sandbox/xwiki-core-rest/src/main/java/components/org/xwiki/rest/ XWikiResourceFinder.java 2009-01-06 14:29:26 UTC (rev 15095) +++ sandbox/xwiki-core-rest/src/main/java/components/org/xwiki/rest/ XWikiResourceFinder.java 2009-01-06 15:43:28 UTC (rev 15096) @@ -1,32 +0,0 @@ -package components.org.xwiki.rest; - -import org.restlet.Context; -import org.restlet.Finder; -import org.restlet.Handler; -import org.restlet.data.Request; -import org.restlet.data.Response; - -/** - * @version $Id$ - */ -public class XWikiResourceFinder extends Finder -{ - private Context context; - - private XWikiResource resource; - - public XWikiResourceFinder(Context context, XWikiResource resource) - { - this.context = context; - this.resource = resource; - } - - @Override - protected Handler findTarget(Request request, Response response) - { - resource.init(context, request, response); - - return resource; - } - -}
Modified: sandbox/xwiki-core-rest/src/main/java/components/org/xwiki/ rest/XWikiRestApplication.java =================================================================== --- sandbox/xwiki-core-rest/src/main/java/components/org/xwiki/rest/ XWikiRestApplication.java 2009-01-06 14:29:26 UTC (rev 15095) +++ sandbox/xwiki-core-rest/src/main/java/components/org/xwiki/rest/ XWikiRestApplication.java 2009-01-06 15:43:28 UTC (rev 15096) @@ -20,7 +20,7 @@ /* Injected by the component manager */ private ComponentManager componentManager;
- private Map<String, XWikiResource> uriPatternToResourceMap; + private Map<String, XWikiResource> classNameToResourceMap;
public XWikiRestApplication() { @@ -48,18 +48,18 @@ getTunnelService().setEnabled(true); getTunnelService().setExtensionsTunnel(true);
- Router router = new Router(getContext()); + Router router = new XWikiRouter(componentManager, getContext());
/* Register all the resource components */ - for (String uriPattern : uriPatternToResourceMap.keySet()) { - XWikiResource resource = uriPatternToResourceMap.get(uriPattern); + for (String className : classNameToResourceMap.keySet()) { + XWikiResource resource = classNameToResourceMap.get(className);
getLogger().log( Level.INFO, - String - .format("Registering resource %s for URI pattern '%s'", resource.getClass().getName(), uriPattern)); + String.format("Attaching resource %s to URI pattern '%s'", resource.getClass().getName(), resource + .getUriPattern()));
- router.attach(uriPattern, new XWikiResourceFinder(getContext(), resource)); + router.attach(resource.getUriPattern(), resource.getClass()); }
return router; @@ -69,4 +69,5 @@ { this.componentManager = componentManager; } + }
Added: sandbox/xwiki-core-rest/src/main/java/components/org/xwiki/ rest/XWikiRouter.java =================================================================== --- sandbox/xwiki-core-rest/src/main/java/components/org/xwiki/rest/ XWikiRouter.java (rev 0) +++ sandbox/xwiki-core-rest/src/main/java/components/org/xwiki/rest/ XWikiRouter.java 2009-01-06 15:43:28 UTC (rev 15096) @@ -0,0 +1,56 @@ +package components.org.xwiki.rest; + +import org.restlet.Context; +import org.restlet.Finder; +import org.restlet.Handler; +import org.restlet.Router; +import org.restlet.data.Request; +import org.restlet.data.Response; +import org.restlet.resource.Resource; +import org.xwiki.component.manager.ComponentLookupException; +import org.xwiki.component.manager.ComponentManager; + +public class XWikiRouter extends Router +{ + private ComponentManager componentManager; + + private static class XWikiFinder extends Finder + { + private ComponentManager componentManager; + + public XWikiFinder(ComponentManager componentManager, Context context, Class< ? extends Handler> targetClass) + { + super(context, targetClass); + this.componentManager = componentManager; + } + + @Override + public Handler createTarget(Class< ? extends Handler> targetClass, Request request, Response response) + { + Handler handler = null; + + try { + handler = (Handler) componentManager.lookup(XWikiResource.class.getName(), targetClass.getName()); + handler.init(getContext(), request, response); + } catch (ComponentLookupException e) { + e.printStackTrace(); + } + + return handler; + } + + } + + public XWikiRouter(ComponentManager componentManager, Context context) + { + super(context); + this.componentManager = componentManager; + } + + @Override + protected Finder createFinder(Class< ? extends Resource> targetClass) + { + return new XWikiFinder(componentManager, getContext(), targetClass); + } + +}
Modified: sandbox/xwiki-core-rest/src/main/resources/META-INF/plexus/ components.xml =================================================================== --- sandbox/xwiki-core-rest/src/main/resources/META-INF/plexus/ components.xml 2009-01-06 14:29:26 UTC (rev 15095) +++ sandbox/xwiki-core-rest/src/main/resources/META-INF/plexus/ components.xml 2009-01-06 15:43:28 UTC (rev 15096) @@ -12,19 +12,23 @@ <requirement> <role>components.org.xwiki.rest.XWikiResource </role> - <field-name>uriPatternToResourceMap</field-name> + <field-name>classNameToResourceMap + </field-name> </requirement> </requirements> </component> + <component> <role>components.org.xwiki.rest.XWikiResource </role> <!-- - The role hint is used to specify the URI template this resource is - attached to. + Role hint must be the same as the implementation since it is used to + lookup for the implementation in XWikiRouter.XWikiFinder. --> - <role-hint>/spaces</role-hint> - <instantiation-strategy>singleton</instantiation-strategy> + <role-hint>components.org.xwiki.rest.SpacesResource + </role-hint> + <!-- Resource instantiation strategy should be per-lookup --> + <instantiation-strategy>per-lookup</instantiation-strategy> <implementation>components.org.xwiki.rest.SpacesResource </implementation> <requirements> @@ -33,10 +37,15 @@ </role> <role-hint>components.org.xwiki.rest.SpacesResource </role-hint> - <field-name>descriptorToRepresenterMap</field-name> + <field-name>descriptorToRepresenterMap + </field-name> </requirement> </requirements> + <configuration> + <uriPattern>/spaces</uriPattern> + </configuration> </component> + <component> <role>components.org.xwiki.rest.XWikiResourceRepresenter </role> @@ -53,7 +62,7 @@ --> <role-hint>components.org.xwiki.rest.SpacesResource:text/plain </role-hint> - <instantiation-strategy>singleton</instantiation-strategy> + <instantiation-strategy>per-lookup</instantiation-strategy> <implementation> components.org.xwiki.rest.internal.PlainTextSpacesRepresenter </implementation> @@ -63,7 +72,7 @@ </role> <role-hint>components.org.xwiki.rest.SpacesResource:text/xml </role-hint> - <instantiation-strategy>singleton</instantiation-strategy> + <instantiation-strategy>per-lookup</instantiation-strategy> <implementation> components.org.xwiki.rest.internal.XmlTextSpacesRepresenter </implementation>
Vincent Massol wrote:
Hi Fabio,
I can't see the reply you got on the list.
The Tigris platform is not good at all. Anyway it's here: http://restlet.tigris.org/ds/getDSMessageAttachment.do/attachment-1305329?ds...
Also I think you've introduced an important bug: whenever plexus instantiates a per-lookup component a new instance is created but you have to release it explicitely as otherwise it won't be released. Which means you'll get an out of memory exception very quickly.
per-look must be avoided as much as possible (we don't have any in the other components of xwiki because of the release problem).
Ok I see. Well, here we don't have the choice so I am going to try to put in place some mechanism to release resource objects.
Also I don't understand why something like XmlTextSpacesRepresenter should not be a singleton.
They can be singletons provided that they are thread-safe. Taking into account what you said before, I am going to declare them as singletons. -Fabio
On Jan 6, 2009, at 5:17 PM, Fabio Mancinelli wrote:
Vincent Massol wrote:
Hi Fabio,
I can't see the reply you got on the list.
The Tigris platform is not good at all. Anyway it's here: http://restlet.tigris.org/ds/getDSMessageAttachment.do/attachment-1305329?ds...
Also I think you've introduced an important bug: whenever plexus instantiates a per-lookup component a new instance is created but you have to release it explicitely as otherwise it won't be released. Which means you'll get an out of memory exception very quickly.
per-look must be avoided as much as possible (we don't have any in the other components of xwiki because of the release problem).
Ok I see. Well, here we don't have the choice so I am going to try to put in place some mechanism to release resource objects.
ok be very very careful with this. We've been beaten already in the past by this and it took me several days to find out the problem... Are we storing any state in the resource components?
Also I don't understand why something like XmlTextSpacesRepresenter should not be a singleton.
They can be singletons provided that they are thread-safe. Taking into account what you said before, I am going to declare them as singletons.
-Fabio
Thanks -Vincent http://xwiki.com http://massol.net http://xwiki.org
Vincent Massol wrote:
Ok I see. Well, here we don't have the choice so I am going to try to put in place some mechanism to release resource objects.
ok be very very careful with this. We've been beaten already in the past by this and it took me several days to find out the problem...
Ok I did it. I put a filter that cleans things up (i.e., release the component) after each request. The object is passed to the filter through the request's attributes. If the resource throws an unhandled exception then the filter is not called. However a custom StatusService can catch this exception and perform the cleanup. In this way a resource component is instantiated and released at each request.
Are we storing any state in the resource components?
Normally we don't but Restlet might. I don't know. -Fabio
participants (2)
-
Fabio Mancinelli -
Vincent Massol