On reflection, both your and my solution have a race. the size method, is a clear check-then-act
the read-only method uses Collections.unmodifiableSet() which only decorates the underlying set, thus is still check-thern-act (the current implementation does not have a race condition, as the data is copied, thus the size will match the data) There is no pleasant way to solve this that I can see. This is my best attempt: 1) Add a new field private static final CopyOnWriteArrayList<String> ZONE_IDS; 2) At the end of registerProvider0() add all of the new IDs to the list (must be outside the loop as otherwise CopyOnWriteArrayList will be slow) 3) Add a new method getAvailableZoneIdList() that returns the list wrapped in Collections.unmodifiableList() 4) In the calling code, query the size, and then use subList(0, size) to lock the size from race conditions. A variation would be 1) Add a new field private static volatile Set<String> ZONE_IDS; 2) Synchronize/lock registerProvider0() to ensure only one thread is in there at a time. 3) At the end of registerProvider0() add all of the new IDs to the set, storing Collections.unmodifiableSet(combinedSet) 4) Add a new method getAvailableZoneIdSet() that returns the unmodifiable set 5) Change the calling code to use the new method, but no other changes Stephen On 5 May 2016 at 16:53, Roger Riggs <roger.ri...@oracle.com> wrote: > Hi Stephen, > > The aspect of the current implementation that is problematic is the copying > of the set, > its not just single object creation but an entry for every ZoneID. > > Adding a size method exposing some internal state increases the possibility > that > when it is used independently it will be out of sync. Not a big issue, but > one to avoid > when designing an API. > > Exposing a mutable Hashset was probably a mistake but one that cannot be > corrected > now. The optics aren't concerning to me. > > SharedSecrets are much messier and not appropriate. > > Adding a method to provide what was originally needed is a cleaner solution. > > Roger > > > > > On 5/5/2016 11:27 AM, Stephen Colebourne wrote: >> >> I fail to see why adding a new read-only method alongside the existing >> method adds any more value to the API than adding a new size method. >> At least with the size method the API is still sensible - a mutable >> and immutable method alongside each other shouts out that a mistake >> was made. A size method is more subtle about the mistake (plenty of >> APIs have a size method alongside a collection method). >> >> Finally, a read-only method involves object creation, thus has worse >> performance than adding a size method. >> >> The only other alternative is perhaps to use SharedSecrets? I don't >> know what possibilities there are there. If not SharedSecrets, then no >> matter what, we are adding "a trivial method to the public API used >> only for an optimization". >> >> Stephen >> >> >> On 5 May 2016 at 15:23, Roger Riggs <roger.ri...@oracle.com> wrote: >>> >>> Hi Bhanu, >>> >>> Adding a trivial method to the public API used only for an optimization >>> is >>> not a good fix for this issue. >>> >>> A better fix was suggested to add a non-copying read-only version of >>> >>> ZoneRulesProvider.getAvailableZoneIds() >>> >>> Please revise the fix to instead implement and use: >>> >>> /** >>> * Gets a readonly set of available zone IDs. >>> * <p> >>> * These IDs are the string form of a {@link ZoneId}. >>> * >>> * @return a unmodifiable copy of the set of zone IDs, not null >>> */ >>> public static Set<String> getReadOnlyAvailableZoneIds() { >>> return Collections.unmodifiableSet(ZONES.keySet()); >>> } >>> >>> Roger >>> >>> >>> >>> On 5/5/2016 5:10 AM, Bhanu Gopularam wrote: >>> >>> Hi all, >>> >>> >>> >>> Please review fix following issue >>> >>> >>> >>> Bug Id : https://bugs.openjdk.java.net/browse/JDK-8066291 >>> >>> >>> >>> Solution: provided new method to get size of available zone ids >>> >>> >>> >>> webrev : >>> http://cr.openjdk.java.net/~bgopularam/bhanu/JDK-8066291/webrev.00 >>> >>> >>> >>> Thanks, >>> >>> Bhanu >>> >>> >