Hi, Using the current number of ZoneIDs to avoid the recompilation of the cache is a bit weak anyway, though it seems unlikely that a ZoneID would be added and one deleted without being noticed. An alternative would be a API that returned a number that would change every time the set of ZoneIds changed. That would be more suitable both as a new API and as something to trigger updates to the cache. I'd rather see a localized implementation with a simple model. Roger On 5/5/2016 1:43 PM, Stephen Colebourne wrote:
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.Riggs@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.Riggs@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