Adrian Crum wrote: > --- On Wed, 1/13/10, Adam Heath <doo...@brainfood.com> wrote: > >> From: Adam Heath <doo...@brainfood.com> >> Subject: Re: svn commit: r899053 - >> /ofbiz/branches/executioncontext20091231/framework/context/src/org/ofbiz/context/ContextUtil.java >> To: dev@ofbiz.apache.org >> Cc: comm...@ofbiz.apache.org >> Date: Wednesday, January 13, 2010, 8:17 PM >> adri...@apache.org >> wrote: >>> Author: adrianc >>> Date: Thu Jan 14 03:54:23 2010 >>> New Revision: 899053 >>> >>> URL: http://svn.apache.org/viewvc?rev=899053&view=rev >>> Log: >>> Better exception handling - suggested by Adam Heath. >>> >>> Modified: >>> >> >> ofbiz/branches/executioncontext20091231/framework/context/src/org/ofbiz/context/ContextUtil.java >>> Modified: >> ofbiz/branches/executioncontext20091231/framework/context/src/org/ofbiz/context/ContextUtil.java >>> URL: >>> http://svn.apache.org/viewvc/ofbiz/branches/executioncontext20091231/framework/context/src/org/ofbiz/context/ContextUtil.java?rev=899053&r1=899052&r2=899053&view=diff >>> >> ============================================================================== >>> --- >> ofbiz/branches/executioncontext20091231/framework/context/src/org/ofbiz/context/ContextUtil.java >> (original) >>> +++ >> ofbiz/branches/executioncontext20091231/framework/context/src/org/ofbiz/context/ContextUtil.java >> Thu Jan 14 03:54:23 2010 >>> @@ -20,6 +20,7 @@ >>> >>> import static >> org.ofbiz.api.authorization.BasicPermissions.Access; >>> >>> +import java.security.AccessControlException; >>> import java.util.List; >>> >>> import javolution.util.FastList; >>> @@ -48,7 +49,9 @@ >>> try { >>> >> accessController.checkPermission(Access, >> artifactPath); >>> >> resultList.add(webAppInfo); >>> - } catch >> (Exception e) {} >>> + } catch >> (AccessControlException e) { >>> + >> // This exception is expected - do nothing >>> + } >>> >> artifactPath.restoreState(); >> >> And what about the OutOfMemoryException that could be >> thrown by >> resultList.add()? >> >> Anytime you have a push/pop kind of pattern, you must use >> push before >> a try, and pop inside the finally. >> >> foo.push(); >> try { >> code(); >> } finally { >> foo.pop(); >> } > > I really appreciate you taking the time to review my code. In this case, I > think you're making much ado about nothing. The object you are concerned > about is on the stack, so if any exception is thrown it will be discarded.
Hmm. I just switched my checkout to your branch. I'm seeing some things that need to be reviewed. I'm going to need to make some time for this. Initially, ArtifactPath implements Iterator<String>. I would have done that as Iterable<String>, which would then allow ArtifactPath to be used in for(type name: var) constructs. Also, in the same class, getPathAsString has inconsistent used of 'this' when referencing the internal stringBuilder object. Actually, what is the prefered way of making use of 'this'? Should it always be used on local method access? local variable access? Personally, I think it should *not* be used, as the compiler will figure it out for you, and the computer should be used for what it is good for, automation. The static PATH_ROOT variable could get corrupted, if calling code doesn't properly do a saveState/restoreState pair. This is based on my earlier emails in this thread. Aie, this one is bad. getPathAsString uses an instance variable, stringBuilder, then manipulates it's state. This could break in multi-threaded situations, esp. when considering that PATH_ROOT is a shared static. Based on ContextUtil and ArtifactPath, it looks like saveState/restoreState are *always* called. This means that ArtifactPath.stack will *always* be set to a FastList instance. It would be better to remove the null checks, and make stack a final instance variable. The array based ArtifactPath constructor should use ... instead of [], to allow varargs. It's generally bad form have an object take a parameter, then not internalize said parameter. Ie, the array constructor doesn't take ownership of the pathElementArray, thereby allowing calling code to manipulate it. If this is by design, it should be listed as such in the javadocs. Does ArtifactPath support federation? Ie, why not use jndi for this, as it has path/name support stuff.
