On Wed, May 6, 2009 at 9:49 AM, Glen Daniels <g...@thoughtcraft.com> wrote:
> keith chapman wrote: > > Um - we aren't currently returning the actual Map, we're returning a > > clone of > > it (just done manually instead of using clone()). You had suggested > > returning the actual Map itself, which is what I was reacting to > > above. I'm > > not saying the API should go away. > > > > The reason we have implemented a clone is the limitation in the API. If > > the return type was Map we could have returned the allServices map. > > Wouldn't this clone be expensive when there are lots of services > deployed? > > Hm - it seems like you didn't actually get my point. We CAN'T return the > actual allServices map because that would be breaking the abstraction > boundary provided by the class. There is (and should be) a contract about > how you add and remove services - if you don't follow the contract, then > stuff like engaging global modules doesn't happen. In our case this stuff > lives in the code in addServiceGroup() (whether or not that's where it > should > live is debatable, but the point is that it exists). If someone were to > mess > around with the actual contents of allServices, that would produce > undefined > results. > > This is basic OOP - don't expose the inner workings of your class by > offering > mutable references to private data. > > Instead of cloning, we could just use Collections.unmodifiableMap() for > speed, but we'd need to be clear (in the code and the JavaDoc) whether > we're > returning a "moment in time" (i.e. a clone()) or a "live view" (which is I > think what unmodifiableMap() gives you) - i.e. if some other thread deploys > a > service while I have the reference, can I see the new service or not? As I remember this change was done to avoid concurrent modifications to service map[1]. At that time services map was a HashMap and could not fix the issue by changing it to a ConcurrentHashMap since API method returns a HashMap. Currently anyone can get a copy of internal map (I think we can not use clone() method since internal implementation is ConcurrentHashMap and we need to return a HashMap). And if they want to add or remove service they have to use addService and removeService respectively. Keith, if you really need the internal map IMHO best way is to add a new API method. thanks, Amila. [1] http://svn.apache.org/viewvc/webservices/axis2/trunk/java/modules/kernel/src/org/apache/axis2/engine/AxisConfiguration.java?r1=679658&r2=684775 > > > --Glen > -- Amila Suriarachchi WSO2 Inc. blog: http://amilachinthaka.blogspot.com/
