Move common metadata loading logic into MetadataManager, removing synchronization everywhere (#1411)

java/libphonenumber/src/com/google/i18n/phonenumbers/MetadataManager.java

@@ -24,48 +24,187 @@ import java.io.InputStream;
 import java.io.ObjectInputStream;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicReference;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
 /**
- * Class encapsulating loading of PhoneNumber Metadata information. Currently this is used only for
- * additional data files such as PhoneNumberAlternateFormats, but in the future it is envisaged it
- * would handle the main metadata file (PhoneNumberMetadata.xml) as well.
+ * Manager for loading metadata for alternate formats and short numbers. We also declare some
+ * constants for phone number metadata loading, to more easily maintain all three types of metadata
+ * together.
+ * TODO: Consider managing phone number metadata loading here too.
  */
 final class MetadataManager {
+  static final String MULTI_FILE_PHONE_NUMBER_METADATA_FILE_PREFIX =
+      "/com/google/i18n/phonenumbers/data/PhoneNumberMetadataProto";
+  static final String SINGLE_FILE_PHONE_NUMBER_METADATA_FILE_NAME =
+      "/com/google/i18n/phonenumbers/data/SingleFilePhoneNumberMetadataProto";
   private static final String ALTERNATE_FORMATS_FILE_PREFIX =
       "/com/google/i18n/phonenumbers/data/PhoneNumberAlternateFormatsProto";
   private static final String SHORT_NUMBER_METADATA_FILE_PREFIX =
       "/com/google/i18n/phonenumbers/data/ShortNumberMetadataProto";
 
+  private static final MetadataLoader METADATA_LOADER = new MetadataLoader() {
+    @Override
+    public InputStream loadMetadata(String metadataFileName) {
+      return MetadataManager.class.getResourceAsStream(metadataFileName);
+    }
+  };
+
   private static final Logger logger = Logger.getLogger(MetadataManager.class.getName());
 
-  private static final Map<Integer, PhoneMetadata> callingCodeToAlternateFormatsMap =
-      Collections.synchronizedMap(new HashMap<Integer, PhoneMetadata>());
-  private static final Map<String, PhoneMetadata> regionCodeToShortNumberMetadataMap =
-      Collections.synchronizedMap(new HashMap<String, PhoneMetadata>());
+  // A mapping from a country calling code to the alternate formats for that country calling code.
+  private static final ConcurrentHashMap<Integer, PhoneMetadata> alternateFormatsMap =
+      new ConcurrentHashMap<Integer, PhoneMetadata>();
+
+  // A mapping from a region code to the short number metadata for that region code.
+  private static final ConcurrentHashMap<String, PhoneMetadata> shortNumberMetadataMap =
+      new ConcurrentHashMap<String, PhoneMetadata>();
 
-  // A set of which country calling codes there are alternate format data for. If the set has an
-  // entry for a code, then there should be data for that code linked into the resources.
-  private static final Set<Integer> countryCodeSet =
+  // The set of country calling codes for which there are alternate formats. For every country
+  // calling code in this set there should be metadata linked into the resources.
+  private static final Set<Integer> alternateFormatsCountryCodes =
       AlternateFormatsCountryCodeSet.getCountryCodeSet();
 
-  // A set of which region codes there are short number data for. If the set has an entry for a
-  // code, then there should be data for that code linked into the resources.
-  private static final Set<String> regionCodeSet = ShortNumbersRegionCodeSet.getRegionCodeSet();
+  // The set of region codes for which there are short number metadata. For every region code in
+  // this set there should be metadata linked into the resources.
+  private static final Set<String> shortNumberMetadataRegionCodes =
+      ShortNumbersRegionCodeSet.getRegionCodeSet();
+
+  private MetadataManager() {}
+
+  static PhoneMetadata getAlternateFormatsForCountry(int countryCallingCode) {
+    if (!alternateFormatsCountryCodes.contains(countryCallingCode)) {
+      return null;
+    }
+    return getMultiFileMetadata(countryCallingCode, alternateFormatsMap,
+        ALTERNATE_FORMATS_FILE_PREFIX, METADATA_LOADER);
+  }
+
+  static PhoneMetadata getShortNumberMetadataForRegion(String regionCode) {
+    if (!shortNumberMetadataRegionCodes.contains(regionCode)) {
+      return null;
+    }
+    return getMultiFileMetadata(regionCode, shortNumberMetadataMap,
+        SHORT_NUMBER_METADATA_FILE_PREFIX, METADATA_LOADER);
+  }
+
+  static Set<String> getSupportedShortNumberRegions() {
+    return Collections.unmodifiableSet(shortNumberMetadataRegionCodes);
+  }
+
+  /**
+   * @param key  the lookup key for the provided map, typically a region code or a country calling
+   *     code
+   * @param map  the map containing mappings of already loaded metadata from their {@code key}. If
+   *     this {@code key}'s metadata isn't already loaded, it will be added to this map after
+   *     loading
+   * @param filePrefix  the prefix of the file to load metadata from
+   * @param metadataLoader  the metadata loader used to inject alternative metadata sources
+   */
+  static <T> PhoneMetadata getMultiFileMetadata(T key, ConcurrentHashMap<T, PhoneMetadata> map,
+      String filePrefix, MetadataLoader metadataLoader) {
+    PhoneMetadata metadata = map.get(key);
+    if (metadata != null) {
+      return metadata;
+    }
+    // We assume key.toString() is well-defined.
+    String fileName = filePrefix + "_" + key;
+    List<PhoneMetadata> metadataList = getMetadataFromFileName(fileName, metadataLoader);
+    if (metadataList.size() > 1) {
+      logger.log(Level.WARNING, "more than one metadata in file " + fileName);
+    }
+    metadata = metadataList.get(0);
+    PhoneMetadata oldValue = map.putIfAbsent(key, metadata);
+    return (oldValue != null) ? oldValue : metadata;
+  }
+
+  // Loader and holder for the metadata maps loaded from a single file.
+  static class SingleFileMetadataMaps {
+    static SingleFileMetadataMaps load(String fileName, MetadataLoader metadataLoader) {
+      List<PhoneMetadata> metadataList = getMetadataFromFileName(fileName, metadataLoader);
+      Map<String, PhoneMetadata> regionCodeToMetadata = new HashMap<String, PhoneMetadata>();
+      Map<Integer, PhoneMetadata> countryCallingCodeToMetadata = new HashMap<Integer, PhoneMetadata>();
+      for (PhoneMetadata metadata : metadataList) {
+        String regionCode = metadata.getId();
+        if (PhoneNumberUtil.REGION_CODE_FOR_NON_GEO_ENTITY.equals(regionCode)) {
+          // regionCode belongs to a non-geographical entity.
+          countryCallingCodeToMetadata.put(metadata.getCountryCode(), metadata);
+        } else {
+          regionCodeToMetadata.put(regionCode, metadata);
+        }
+      }
+      return new SingleFileMetadataMaps(regionCodeToMetadata, countryCallingCodeToMetadata);
+    }
+
+    // A map from a region code to the PhoneMetadata for that region.
+    // For phone number metadata, the region code "001" is excluded, since that is used for the
+    // non-geographical phone number entities.
+    private final Map<String, PhoneMetadata> regionCodeToMetadata;
+
+    // A map from a country calling code to the PhoneMetadata for that country calling code.
+    // Examples of the country calling codes include 800 (International Toll Free Service) and 808
+    // (International Shared Cost Service).
+    // For phone number metadata, only the non-geographical phone number entities' country calling
+    // codes are present.
+    private final Map<Integer, PhoneMetadata> countryCallingCodeToMetadata;
+
+    private SingleFileMetadataMaps(Map<String, PhoneMetadata> regionCodeToMetadata,
+        Map<Integer, PhoneMetadata> countryCallingCodeToMetadata) {
+      this.regionCodeToMetadata = Collections.unmodifiableMap(regionCodeToMetadata);
+      this.countryCallingCodeToMetadata = Collections.unmodifiableMap(countryCallingCodeToMetadata);
+    }
+
+    PhoneMetadata get(String regionCode) {
+      return regionCodeToMetadata.get(regionCode);
+    }
+
+    PhoneMetadata get(int countryCallingCode) {
+      return countryCallingCodeToMetadata.get(countryCallingCode);
+    }
+  }
+
+  // Manages the atomic reference lifecycle of a SingleFileMetadataMaps encapsulation.
+  static SingleFileMetadataMaps getSingleFileMetadataMaps(
+      AtomicReference<SingleFileMetadataMaps> ref, String fileName, MetadataLoader metadataLoader) {
+    SingleFileMetadataMaps maps = ref.get();
+    if (maps == null) {
+      maps = SingleFileMetadataMaps.load(fileName, metadataLoader);
+      SingleFileMetadataMaps existingValue = ref.getAndSet(maps);
+      if (existingValue != null) {
+        maps = existingValue;
+      }
+    }
+    return maps;
+  }
 
-  private MetadataManager() {
+  private static List<PhoneMetadata> getMetadataFromFileName(String fileName,
+      MetadataLoader metadataLoader) {
+    InputStream source = metadataLoader.loadMetadata(fileName);
+    if (source == null) {
+      // Sanity check; this would only happen if we packaged jars incorrectly.
+      throw new IllegalStateException("missing metadata: " + fileName);
+    }
+    PhoneMetadataCollection metadataCollection = loadMetadataAndCloseInput(source);
+    List<PhoneMetadata> metadataList = metadataCollection.getMetadataList();
+    if (metadataList.size() == 0) {
+      // Sanity check; this should not happen since we build with non-empty metadata.
+      throw new IllegalStateException("empty metadata: " + fileName);
+    }
+    return metadataList;
   }
 
   /**
-   * Loads and returns the metadata object from the given stream and closes the stream.
+   * Loads and returns the metadata from the given stream and closes the stream.
    *
    * @param source  the non-null stream from which metadata is to be read
-   * @return  the loaded metadata object
+   * @return  the loaded metadata
    */
-  static PhoneMetadataCollection loadMetadataAndCloseInput(InputStream source) {
+  private static PhoneMetadataCollection loadMetadataAndCloseInput(InputStream source) {
     ObjectInputStream ois = null;
     try {
       try {
@@ -93,61 +232,4 @@ final class MetadataManager {
       }
     }
   }
-
-  private static void loadAlternateFormatsMetadataFromFile(int countryCallingCode) {
-    String fileName = ALTERNATE_FORMATS_FILE_PREFIX + "_" + countryCallingCode;
-    InputStream source = MetadataManager.class.getResourceAsStream(fileName);
-    if (source == null) {
-      // Sanity check; this should not happen since we only load things based on the expectation
-      // that they are present, by checking the map of available data first.
-      throw new IllegalStateException("missing metadata: " + fileName);
-    }
-    PhoneMetadataCollection alternateFormats = loadMetadataAndCloseInput(source);
-    for (PhoneMetadata metadata : alternateFormats.getMetadataList()) {
-      callingCodeToAlternateFormatsMap.put(metadata.getCountryCode(), metadata);
-    }
-  }
-
-  static PhoneMetadata getAlternateFormatsForCountry(int countryCallingCode) {
-    if (!countryCodeSet.contains(countryCallingCode)) {
-      return null;
-    }
-    synchronized (callingCodeToAlternateFormatsMap) {
-      if (!callingCodeToAlternateFormatsMap.containsKey(countryCallingCode)) {
-        loadAlternateFormatsMetadataFromFile(countryCallingCode);
-      }
-    }
-    return callingCodeToAlternateFormatsMap.get(countryCallingCode);
-  }
-
-  private static void loadShortNumberMetadataFromFile(String regionCode) {
-    String fileName = SHORT_NUMBER_METADATA_FILE_PREFIX + "_" + regionCode;
-    InputStream source = MetadataManager.class.getResourceAsStream(fileName);
-    if (source == null) {
-      // Sanity check; this should not happen since we only load things based on the expectation
-      // that they are present, by checking the map of available data first.
-      throw new IllegalStateException("missing metadata: " + fileName);
-    }
-    PhoneMetadataCollection shortNumberMetadata = loadMetadataAndCloseInput(source);
-    for (PhoneMetadata metadata : shortNumberMetadata.getMetadataList()) {
-      regionCodeToShortNumberMetadataMap.put(regionCode, metadata);
-    }
-  }
-
-  // @VisibleForTesting
-  static Set<String> getShortNumberMetadataSupportedRegions() {
-    return regionCodeSet;
-  }
-
-  static PhoneMetadata getShortNumberMetadataForRegion(String regionCode) {
-    if (!regionCodeSet.contains(regionCode)) {
-      return null;
-    }
-    synchronized (regionCodeToShortNumberMetadataMap) {
-      if (!regionCodeToShortNumberMetadataMap.containsKey(regionCode)) {
-        loadShortNumberMetadataFromFile(regionCode);
-      }
-    }
-    return regionCodeToShortNumberMetadataMap.get(regionCode);
-  }
 }

java/libphonenumber/src/com/google/i18n/phonenumbers/MultiFileMetadataSourceImpl.java

@@ -17,74 +17,63 @@
 package com.google.i18n.phonenumbers;
 
 import com.google.i18n.phonenumbers.Phonemetadata.PhoneMetadata;
-import com.google.i18n.phonenumbers.Phonemetadata.PhoneMetadataCollection;
-
-import java.io.InputStream;
 import java.util.List;
 import java.util.concurrent.ConcurrentHashMap;
-import java.util.logging.Level;
-import java.util.logging.Logger;
 
 /**
  * Implementation of {@link MetadataSource} that reads from multiple resource files.
  */
 final class MultiFileMetadataSourceImpl implements MetadataSource {
+  // The prefix of the binary files containing phone number metadata for different regions.
+  // This enables us to set up with different metadata, such as for testing.
+  private final String multiFilePhoneNumberMetadataFilePrefix;
 
-  private static final Logger logger =
-      Logger.getLogger(MultiFileMetadataSourceImpl.class.getName());
-
-  private static final String META_DATA_FILE_PREFIX =
-      "/com/google/i18n/phonenumbers/data/PhoneNumberMetadataProto";
+  // The {@link MetadataLoader} used to inject alternative metadata sources.
+  private final MetadataLoader metadataLoader;
 
-  // A mapping from a region code to the PhoneMetadata for that region.
+  // A mapping from a region code to the phone number metadata for that region code.
+  // Unlike the mappings for alternate formats and short number metadata, the phone number metadata
+  // is loaded from a non-statically determined file prefix; therefore this map is bound to the
+  // instance and not static.
   private final ConcurrentHashMap<String, PhoneMetadata> geographicalRegions =
       new ConcurrentHashMap<String, PhoneMetadata>();
 
-  // A mapping from a country calling code for a non-geographical entity to the PhoneMetadata for
-  // that country calling code. Examples of the country calling codes include 800 (International
-  // Toll Free Service) and 808 (International Shared Cost Service).
+  // A mapping from a country calling code for a non-geographical entity to the phone number
+  // metadata for that country calling code. Examples of the country calling codes include 800
+  // (International Toll Free Service) and 808 (International Shared Cost Service).
+  // Unlike the mappings for alternate formats and short number metadata, the phone number metadata
+  // is loaded from a non-statically determined file prefix; therefore this map is bound to the
+  // instance and not static.
   private final ConcurrentHashMap<Integer, PhoneMetadata> nonGeographicalRegions =
       new ConcurrentHashMap<Integer, PhoneMetadata>();
 
-  // The prefix of the metadata files from which region data is loaded.
-  private final String filePrefix;
-
-  // The metadata loader used to inject alternative metadata sources.
-  private final MetadataLoader metadataLoader;
-
-  // It is assumed that metadataLoader is not null. If needed, checks should happen before passing
-  // here.
+  // It is assumed that metadataLoader is not null. Checks should happen before passing here.
   // @VisibleForTesting
-  MultiFileMetadataSourceImpl(String filePrefix, MetadataLoader metadataLoader) {
-    this.filePrefix = filePrefix;
+  MultiFileMetadataSourceImpl(String multiFilePhoneNumberMetadataFilePrefix,
+      MetadataLoader metadataLoader) {
+    this.multiFilePhoneNumberMetadataFilePrefix = multiFilePhoneNumberMetadataFilePrefix;
     this.metadataLoader = metadataLoader;
   }
 
-  // It is assumed that metadataLoader is not null. If needed, checks should happen before passing
-  // here.
+  // It is assumed that metadataLoader is not null. Checks should happen before passing here.
   public MultiFileMetadataSourceImpl(MetadataLoader metadataLoader) {
-    this(META_DATA_FILE_PREFIX, metadataLoader);
+    this(MetadataManager.MULTI_FILE_PHONE_NUMBER_METADATA_FILE_PREFIX, metadataLoader);
   }
 
   @Override
   public PhoneMetadata getMetadataForRegion(String regionCode) {
-    PhoneMetadata metadata = geographicalRegions.get(regionCode);
-    return (metadata != null) ? metadata : loadMetadataFromFile(
-        regionCode, geographicalRegions, filePrefix, metadataLoader);
+    return MetadataManager.getMultiFileMetadata(regionCode, geographicalRegions,
+        multiFilePhoneNumberMetadataFilePrefix, metadataLoader);
   }
 
   @Override
   public PhoneMetadata getMetadataForNonGeographicalRegion(int countryCallingCode) {
-    PhoneMetadata metadata = nonGeographicalRegions.get(countryCallingCode);
-    if (metadata != null) {
-      return metadata;
-    }
-    if (isNonGeographical(countryCallingCode)) {
-      return loadMetadataFromFile(
-          countryCallingCode, nonGeographicalRegions, filePrefix, metadataLoader);
+    if (!isNonGeographical(countryCallingCode)) {
+      // The given country calling code was for a geographical region.
+      return null;
     }
-    // The given country calling code was for a geographical region.
-    return null;
+    return MetadataManager.getMultiFileMetadata(countryCallingCode, nonGeographicalRegions,
+        multiFilePhoneNumberMetadataFilePrefix, metadataLoader);
   }
 
   // A country calling code is non-geographical if it only maps to the non-geographical region code,
@@ -95,38 +84,4 @@ final class MultiFileMetadataSourceImpl implements MetadataSource {
     return (regionCodes.size() == 1
         && PhoneNumberUtil.REGION_CODE_FOR_NON_GEO_ENTITY.equals(regionCodes.get(0)));
   }
-
-  /**
-   * @param key             The geographical region code or the non-geographical region's country
-   *                        calling code.
-   * @param map             The map to contain the mapping from {@code key} to the corresponding
-   *                        metadata.
-   * @param filePrefix      The prefix of the metadata files from which region data is loaded.
-   * @param metadataLoader  The metadata loader used to inject alternative metadata sources.
-   */
-  // @VisibleForTesting
-  static <T> PhoneMetadata loadMetadataFromFile(
-      T key, ConcurrentHashMap<T, PhoneMetadata> map, String filePrefix,
-      MetadataLoader metadataLoader) {
-    // We assume key.toString() is well-defined.
-    String fileName = filePrefix + "_" + key;
-    InputStream source = metadataLoader.loadMetadata(fileName);
-    if (source == null) {
-      // Sanity check; this should not happen since we only load things based on the expectation
-      // that they are present, by checking the map of available data first.
-      throw new IllegalStateException("missing metadata: " + fileName);
-    }
-    PhoneMetadataCollection metadataCollection = MetadataManager.loadMetadataAndCloseInput(source);
-    List<PhoneMetadata> metadataList = metadataCollection.getMetadataList();
-    if (metadataList.isEmpty()) {
-      // Sanity check; this should not happen since we build with non-empty metadata.
-      throw new IllegalStateException("empty metadata: " + fileName);
-    }
-    if (metadataList.size() > 1) {
-      logger.log(Level.WARNING, "invalid metadata (too many entries): " + fileName);
-    }
-    PhoneMetadata metadata = metadataList.get(0);
-    PhoneMetadata oldValue = map.putIfAbsent(key, metadata);
-    return (oldValue != null) ? oldValue : metadata;
-  }
 }

java/libphonenumber/src/com/google/i18n/phonenumbers/ShortNumberInfo.java

@@ -454,7 +454,7 @@ public class ShortNumberInfo {
    * Convenience method to get a list of what regions the library has metadata for.
    */
   Set<String> getSupportedRegions() {
-    return Collections.unmodifiableSet(MetadataManager.getShortNumberMetadataSupportedRegions());
+    return MetadataManager.getSupportedShortNumberRegions();
   }
 
   /**

java/libphonenumber/src/com/google/i18n/phonenumbers/SingleFileMetadataSourceImpl.java

@@ -17,105 +17,49 @@
 package com.google.i18n.phonenumbers;
 
 import com.google.i18n.phonenumbers.Phonemetadata.PhoneMetadata;
-import com.google.i18n.phonenumbers.Phonemetadata.PhoneMetadataCollection;
-
-import java.io.InputStream;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.logging.Logger;
+import java.util.concurrent.atomic.AtomicReference;
 
 /**
  * Implementation of {@link MetadataSource} that reads from a single resource file.
  */
 final class SingleFileMetadataSourceImpl implements MetadataSource {
+  // The name of the binary file containing phone number metadata for different regions.
+  // This enables us to set up with different metadata, such as for testing.
+  private final String phoneNumberMetadataFileName;
 
-  private static final Logger logger =
-      Logger.getLogger(SingleFileMetadataSourceImpl.class.getName());
-
-  private static final String META_DATA_FILE_NAME =
-      "/com/google/i18n/phonenumbers/data/SingleFilePhoneNumberMetadataProto";
-
-  // A mapping from a region code to the PhoneMetadata for that region.
-  // Note: Synchronization, though only needed for the Android version of the library, is used in
-  // all versions for consistency.
-  private final Map<String, PhoneMetadata> regionToMetadataMap =
-      Collections.synchronizedMap(new HashMap<String, PhoneMetadata>());
-
-  // A mapping from a country calling code for a non-geographical entity to the PhoneMetadata for
-  // that country calling code. Examples of the country calling codes include 800 (International
-  // Toll Free Service) and 808 (International Shared Cost Service).
-  // Note: Synchronization, though only needed for the Android version of the library, is used in
-  // all versions for consistency.
-  private final Map<Integer, PhoneMetadata> countryCodeToNonGeographicalMetadataMap =
-      Collections.synchronizedMap(new HashMap<Integer, PhoneMetadata>());
-
-  // The metadata file from which region data is loaded.
-  private final String fileName;
-
-  // The metadata loader used to inject alternative metadata sources.
+  // The {@link MetadataLoader} used to inject alternative metadata sources.
   private final MetadataLoader metadataLoader;
 
-  // It is assumed that metadataLoader is not null.
-  public SingleFileMetadataSourceImpl(String fileName, MetadataLoader metadataLoader) {
-    this.fileName = fileName;
+  private final AtomicReference<MetadataManager.SingleFileMetadataMaps> phoneNumberMetadataRef =
+      new AtomicReference<MetadataManager.SingleFileMetadataMaps>();
+
+  // It is assumed that metadataLoader is not null. Checks should happen before passing it in here.
+  // @VisibleForTesting
+  SingleFileMetadataSourceImpl(String phoneNumberMetadataFileName, MetadataLoader metadataLoader) {
+    this.phoneNumberMetadataFileName = phoneNumberMetadataFileName;
     this.metadataLoader = metadataLoader;
   }
 
-  // It is assumed that metadataLoader is not null.
+  // It is assumed that metadataLoader is not null. Checks should happen before passing it in here.
   public SingleFileMetadataSourceImpl(MetadataLoader metadataLoader) {
-    this(META_DATA_FILE_NAME, metadataLoader);
+    this(MetadataManager.SINGLE_FILE_PHONE_NUMBER_METADATA_FILE_NAME, metadataLoader);
   }
 
   @Override
   public PhoneMetadata getMetadataForRegion(String regionCode) {
-    synchronized (regionToMetadataMap) {
-      if (!regionToMetadataMap.containsKey(regionCode)) {
-        // The regionCode here will be valid and won't be '001', so we don't need to worry about
-        // what to pass in for the country calling code.
-        loadMetadataFromFile();
-      }
-    }
-    return regionToMetadataMap.get(regionCode);
+    return MetadataManager.getSingleFileMetadataMaps(phoneNumberMetadataRef,
+        phoneNumberMetadataFileName, metadataLoader).get(regionCode);
   }
 
   @Override
   public PhoneMetadata getMetadataForNonGeographicalRegion(int countryCallingCode) {
-    synchronized (countryCodeToNonGeographicalMetadataMap) {
-      if (!countryCodeToNonGeographicalMetadataMap.containsKey(countryCallingCode)) {
-        loadMetadataFromFile();
-      }
-    }
-    return countryCodeToNonGeographicalMetadataMap.get(countryCallingCode);
-  }
-
-  // @VisibleForTesting
-  void loadMetadataFromFile() {
-    InputStream source = metadataLoader.loadMetadata(fileName);
-    if (source == null) {
-      // This should not happen since clients shouldn't be using this implementation directly.
-      // The single file implementation is experimental, only for when the jars contain a single
-      // file with all regions' metadata. Currently we do not release such jars.
-      // TODO(b/30807096): Get the MetadataManager to decide whether to use this or the multi file
-      // loading depending on what data is available in the jar.
-      throw new IllegalStateException("missing metadata: " + fileName);
-    }
-    PhoneMetadataCollection metadataCollection = MetadataManager.loadMetadataAndCloseInput(source);
-    List<PhoneMetadata> metadataList = metadataCollection.getMetadataList();
-    if (metadataList.isEmpty()) {
-      // This should not happen since clients shouldn't be using this implementation!
-      throw new IllegalStateException("empty metadata: " + fileName);
-    }
-    for (PhoneMetadata metadata : metadataList) {
-      String regionCode = metadata.getId();
-      int countryCallingCode = metadata.getCountryCode();
-      boolean isNonGeoRegion = PhoneNumberUtil.REGION_CODE_FOR_NON_GEO_ENTITY.equals(regionCode);
-      if (isNonGeoRegion) {
-        countryCodeToNonGeographicalMetadataMap.put(countryCallingCode, metadata);
-      } else {
-        regionToMetadataMap.put(regionCode, metadata);
-      }
-    }
+    // A country calling code is non-geographical if it only maps to the non-geographical region
+    // code, i.e. "001". If this is not true of the given country calling code, then we will return
+    // null here. If not for the atomic reference, such as if we were loading in multiple stages, we
+    // would check that the passed in country calling code was indeed non-geographical to avoid
+    // loading costs for a null result. Here though we do not check this since the entire data must
+    // be loaded anyway if any of it is needed at some point in the life cycle of this class.
+    return MetadataManager.getSingleFileMetadataMaps(phoneNumberMetadataRef,
+        phoneNumberMetadataFileName, metadataLoader).get(countryCallingCode);
   }
 }

java/libphonenumber/test/com/google/i18n/phonenumbers/MetadataManagerTest.java

@@ -17,26 +17,18 @@
 package com.google.i18n.phonenumbers;
 
 import com.google.i18n.phonenumbers.Phonemetadata.PhoneMetadata;
-
+import java.util.concurrent.ConcurrentHashMap;
 import junit.framework.TestCase;
 
 /**
- * Some basic tests to check that the phone number metadata can be correctly loaded.
+ * Some basic tests to check that metadata can be correctly loaded.
  */
-public class MetadataManagerTest extends TestCase {
-
-  public void testAlternateFormatsContainsData() throws Exception {
+public final class MetadataManagerTest extends TestCase {
+  public void testAlternateFormatsLoadCorrectly() {
     // We should have some data for Germany.
-    PhoneMetadata germanyAlternateFormats = MetadataManager.getAlternateFormatsForCountry(49);
-    assertNotNull(germanyAlternateFormats);
-    assertTrue(germanyAlternateFormats.numberFormatSize() > 0);
-  }
-
-  public void testShortNumberMetadataContainsData() throws Exception {
-    // We should have some data for France.
-    PhoneMetadata franceShortNumberMetadata = MetadataManager.getShortNumberMetadataForRegion("FR");
-    assertNotNull(franceShortNumberMetadata);
-    assertTrue(franceShortNumberMetadata.hasShortCode());
+    PhoneMetadata germanyMetadata = MetadataManager.getAlternateFormatsForCountry(49);
+    assertNotNull(germanyMetadata);
+    assertTrue(germanyMetadata.numberFormats().size() > 0);
   }
 
   public void testAlternateFormatsFailsGracefully() throws Exception {
@@ -44,8 +36,51 @@ public class MetadataManagerTest extends TestCase {
     assertNull(noAlternateFormats);
   }
 
+  public void testShortNumberMetadataLoadCorrectly() throws Exception {
+    // We should have some data for France.
+    PhoneMetadata franceMetadata = MetadataManager.getShortNumberMetadataForRegion("FR");
+    assertNotNull(franceMetadata);
+    assertTrue(franceMetadata.hasShortCode());
+  }
+
   public void testShortNumberMetadataFailsGracefully() throws Exception {
     PhoneMetadata noShortNumberMetadata = MetadataManager.getShortNumberMetadataForRegion("XXX");
     assertNull(noShortNumberMetadata);
   }
+
+  public void testGetMultiFileMetadata_regionCode() {
+    ConcurrentHashMap<String, PhoneMetadata> map = new ConcurrentHashMap<String, PhoneMetadata>();
+    PhoneMetadata metadata = MetadataManager.getMultiFileMetadata("CA", map,
+        "/com/google/i18n/phonenumbers/data/PhoneNumberMetadataProtoForTesting",
+        PhoneNumberUtil.DEFAULT_METADATA_LOADER);
+    assertEquals(metadata, map.get("CA"));
+  }
+
+  public void testGetMultiFileMetadata_countryCallingCode() {
+    ConcurrentHashMap<Integer, PhoneMetadata> map = new ConcurrentHashMap<Integer, PhoneMetadata>();
+    PhoneMetadata metadata = MetadataManager.getMultiFileMetadata(800, map,
+        "/com/google/i18n/phonenumbers/data/PhoneNumberMetadataProtoForTesting",
+        PhoneNumberUtil.DEFAULT_METADATA_LOADER);
+    assertEquals(metadata, map.get(800));
+  }
+
+  public void testGetMultiFileMetadata_missingMetadataFileThrowsRuntimeException() {
+    // In normal usage we should never get a state where we are asking to load metadata that doesn't
+    // exist. However if the library is packaged incorrectly in the jar, this could happen and the
+    // best we can do is make sure the exception has the file name in it.
+    try {
+      MetadataManager.getMultiFileMetadata("XX", new ConcurrentHashMap<String, PhoneMetadata>(),
+          "no/such/file", PhoneNumberUtil.DEFAULT_METADATA_LOADER);
+      fail("expected exception");
+    } catch (RuntimeException e) {
+      assertTrue("Unexpected error: " + e, e.getMessage().contains("no/such/file_XX"));
+    }
+    try {
+      MetadataManager.getMultiFileMetadata(123, new ConcurrentHashMap<Integer, PhoneMetadata>(),
+          "no/such/file", PhoneNumberUtil.DEFAULT_METADATA_LOADER);
+      fail("expected exception");
+    } catch (RuntimeException e) {
+      assertTrue("Unexpected error: " + e, e.getMessage().contains("no/such/file_123"));
+    }
+  }
 }

java/libphonenumber/test/com/google/i18n/phonenumbers/MultiFileMetadataSourceImplTest.java

@@ -17,7 +17,6 @@
 package com.google.i18n.phonenumbers;
 
 import com.google.i18n.phonenumbers.Phonemetadata.PhoneMetadata;
-
 import java.util.concurrent.ConcurrentHashMap;
 import junit.framework.TestCase;
 
@@ -25,43 +24,40 @@ import junit.framework.TestCase;
  * Unit tests for MultiFileMetadataSourceImpl.java.
  */
 public class MultiFileMetadataSourceImplTest extends TestCase {
-  public MultiFileMetadataSourceImplTest() {}
-
-  public void testGeographicalRegionMetadataLoadsCorrectly() {
-    ConcurrentHashMap<String, PhoneMetadata> map = new ConcurrentHashMap<String, PhoneMetadata>();
-    PhoneMetadata metadata = MultiFileMetadataSourceImpl.loadMetadataFromFile(
-        "CA", map, "/com/google/i18n/phonenumbers/data/PhoneNumberMetadataProtoForTesting",
-        PhoneNumberUtil.DEFAULT_METADATA_LOADER);
-    assertEquals(metadata, map.get("CA"));
-  }
+  private static final MultiFileMetadataSourceImpl source =
+      new MultiFileMetadataSourceImpl(PhoneNumberUtil.DEFAULT_METADATA_LOADER);
+  private static final MultiFileMetadataSourceImpl missingFileSource =
+      new MultiFileMetadataSourceImpl("no/such/file", PhoneNumberUtil.DEFAULT_METADATA_LOADER);
 
-  public void testNonGeographicalRegionMetadataLoadsCorrectly() {
-    ConcurrentHashMap<Integer, PhoneMetadata> map = new ConcurrentHashMap<Integer, PhoneMetadata>();
-    PhoneMetadata metadata = MultiFileMetadataSourceImpl.loadMetadataFromFile(
-        800, map, "/com/google/i18n/phonenumbers/data/PhoneNumberMetadataProtoForTesting",
-        PhoneNumberUtil.DEFAULT_METADATA_LOADER);
-    assertEquals(metadata, map.get(800));
+  public void testGeoPhoneNumberMetadataLoadCorrectly() {
+    // We should have some data for the UAE.
+    PhoneMetadata uaeMetadata = source.getMetadataForRegion("AE");
+    assertEquals(uaeMetadata.getCountryCode(), 971);
+    assertTrue(uaeMetadata.hasGeneralDesc());
   }
 
-  public void testMissingMetadataFileThrowsRuntimeException() {
-    // In normal usage we should never get a state where we are asking to load metadata that doesn't
-    // exist. However if the library is packaged incorrectly in the jar, this could happen and the
-    // best we can do is make sure the exception has the file name in it.
+  public void testGeoPhoneNumberMetadataLoadFromMissingFileThrowsException() throws Exception {
     try {
-      MultiFileMetadataSourceImpl.loadMetadataFromFile(
-          "XX", new ConcurrentHashMap<String, PhoneMetadata>(), "no/such/file",
-          PhoneNumberUtil.DEFAULT_METADATA_LOADER);
+      missingFileSource.getMetadataForRegion("AE");
       fail("expected exception");
     } catch (RuntimeException e) {
-      assertTrue("Unexpected error: " + e, e.getMessage().contains("no/such/file_XX"));
+      assertTrue("Unexpected error: " + e, e.getMessage().contains("no/such/file"));
     }
+  }
+
+  public void testNonGeoPhoneNumberMetadataLoadCorrectly() {
+    // We should have some data for international toll-free numbers.
+    PhoneMetadata intlMetadata = source.getMetadataForNonGeographicalRegion(800);
+    assertEquals(intlMetadata.getId(), "001");
+    assertTrue(intlMetadata.hasGeneralDesc());
+  }
+
+  public void testNonGeoPhoneNumberMetadataLoadFromMissingFileThrowsException() throws Exception {
     try {
-      MultiFileMetadataSourceImpl.loadMetadataFromFile(
-          123, new ConcurrentHashMap<Integer, PhoneMetadata>(), "no/such/file",
-          PhoneNumberUtil.DEFAULT_METADATA_LOADER);
+      missingFileSource.getMetadataForNonGeographicalRegion(800);
       fail("expected exception");
     } catch (RuntimeException e) {
-      assertTrue("Unexpected error: " + e, e.getMessage().contains("no/such/file_123"));
+      assertTrue("Unexpected error: " + e, e.getMessage().contains("no/such/file"));
     }
   }
 }

java/libphonenumber/test/com/google/i18n/phonenumbers/SingleFileMetadataSourceImplTest.java

@@ -20,18 +20,26 @@ import junit.framework.TestCase;
 
 /**
  * Unit tests for SingleFileMetadataSourceImpl.java.
+ *
+ * <p>
+ * We do not package single file metadata files, so it is only possible to test failures here.
  */
 public class SingleFileMetadataSourceImplTest extends TestCase {
-  public SingleFileMetadataSourceImplTest() {}
+  private static final SingleFileMetadataSourceImpl MISSING_FILE_SOURCE =
+      new SingleFileMetadataSourceImpl("no/such/file", PhoneNumberUtil.DEFAULT_METADATA_LOADER);
+
+  public void testGeoPhoneNumberMetadataLoadFromMissingFileThrowsException() throws Exception {
+    try {
+      MISSING_FILE_SOURCE.getMetadataForRegion("AE");
+      fail("expected exception");
+    } catch (RuntimeException e) {
+      assertTrue("Unexpected error: " + e, e.getMessage().contains("no/such/file"));
+    }
+  }
 
-  public void testMissingMetadataFileThrowsRuntimeException() {
-    SingleFileMetadataSourceImpl singleFileMetadataSource = new SingleFileMetadataSourceImpl(
-        "no/such/file", PhoneNumberUtil.DEFAULT_METADATA_LOADER);
-    // In normal usage we should never get a state where we are asking to load metadata that doesn't
-    // exist. However if the library is packaged incorrectly in the jar, this could happen and the
-    // best we can do is make sure the exception has the file name in it.
+  public void testNonGeoPhoneNumberMetadataLoadFromMissingFileThrowsException() throws Exception {
     try {
-      singleFileMetadataSource.loadMetadataFromFile();
+      MISSING_FILE_SOURCE.getMetadataForNonGeographicalRegion(800);
       fail("expected exception");
     } catch (RuntimeException e) {
       assertTrue("Unexpected error: " + e, e.getMessage().contains("no/such/file"));

java/pending_code_changes.txt

@@ -1,4 +1,7 @@
 Code changes:
- - Removing all references to possible_number_pattern other than the proto buffer
-   itself (and derived files, or hand-crafted files based on it.) This information is
-   no longer present in the binary.
+ - Removing all references to possible_number_pattern other than the proto
+   buffer itself (and derived files, or hand-crafted files based on it.) This
+   information is no longer present in the binary.
+ - Refactored metadata loading and removed synchronization for all kinds of
+   metadata. Clients may experience quicker loading of alternate formats and
+   short number metadata, but no change is required for callers of the library.

tools/java/common/test/com/google/i18n/phonenumbers/MetadataFilterTest.java

@@ -21,7 +21,7 @@ import java.util.TreeMap;
 import java.util.TreeSet;
 import junit.framework.TestCase;
 
-public final class MetadataFilterTest extends TestCase {
+public class MetadataFilterTest extends TestCase {
   // If this behavior changes then consider whether the change in the blacklist is intended, or you
   // should change the special build configuration. Also look into any change in the size of the
   // build.