Avoid synchronization by cloning a prototype Mac and reallocating buffers

Author: jchambers

README.md

@@ -65,9 +65,7 @@ Future password:  046148
 
 ## Performance and best practices
 
-One-time password generators are thread-safe and reusable. Generally, applications should treat one-time password generator instances as long-lived resources.
-
-The password-generating methods in a one-time password generator are `synchronized`. In multi-threaded applications that make heavy use of a shared one-time password generator instance, synchronization may become a performance bottleneck. In that case, callers may benefit from using one instance per thread (for example, by using a [`ThreadLocal`](https://docs.oracle.com/javase/8/docs/api/java/lang/ThreadLocal.html)).
+One-time password generators are thread-safe and reusable. Generally, applications should treat one-time password generator instances as long-lived resources (as opposed to creating new generators for each password-generation call).
 
 ## License and copyright
 

src/main/java/com/eatthepath/otp/HmacOneTimePasswordGenerator.java

@@ -29,22 +29,16 @@
 import java.util.Locale;
 
 /**
- * <p>Generates HMAC-based one-time passwords (HOTP) as specified in
- * <a href="https://tools.ietf.org/html/rfc4226">RFC&nbsp;4226</a>.</p>
- *
- * <p>{@code HmacOneTimePasswordGenerator} instances are thread-safe and may be shared between threads. Note that the
- * {@link #generateOneTimePassword(Key, long)} method (and its relatives) are {@code synchronized}; in multi-threaded
- * applications that make heavy use of a shared {@code HmacOneTimePasswordGenerator} instance, synchronization may
- * become a performance bottleneck. In that case, callers may benefit from using one
- * {@code HmacOneTimePasswordGenerator} instance per thread (for example, with a {@link ThreadLocal}).</p>
+ * Generates HMAC-based one-time passwords (HOTP) as specified in
+ * <a href="https://tools.ietf.org/html/rfc4226">RFC&nbsp;4226</a>. {@code HmacOneTimePasswordGenerator} instances are
+ * thread-safe and may be shared between threads.
  *
  * @author <a href="https://github.com/jchambers">Jon Chambers</a>
  */
 public class HmacOneTimePasswordGenerator {
-    private final Mac mac;
+    private final Mac prototypeMac;
     private final int passwordLength;
 
-    private final ByteBuffer buffer;
     private final int modDivisor;
 
     private final String formatString;
@@ -92,7 +86,7 @@ public HmacOneTimePasswordGenerator(final int passwordLength) {
      */
     HmacOneTimePasswordGenerator(final int passwordLength, final String algorithm) throws UncheckedNoSuchAlgorithmException {
         try {
-            this.mac = Mac.getInstance(algorithm);
+            this.prototypeMac = Mac.getInstance(algorithm);
         } catch (final NoSuchAlgorithmException e) {
             throw new UncheckedNoSuchAlgorithmException(e);
         }
@@ -122,7 +116,6 @@ public HmacOneTimePasswordGenerator(final int passwordLength) {
         }
 
         this.passwordLength = passwordLength;
-        this.buffer = ByteBuffer.allocate(this.mac.getMacLength());
     }
 
     /**
@@ -136,24 +129,41 @@ public HmacOneTimePasswordGenerator(final int passwordLength) {
      *
      * @throws InvalidKeyException if the given key is inappropriate for initializing the {@link Mac} for this generator
      */
-    public synchronized int generateOneTimePassword(final Key key, final long counter) throws InvalidKeyException {
-        this.buffer.clear();
-        this.buffer.putLong(0, counter);
+    public int generateOneTimePassword(final Key key, final long counter) throws InvalidKeyException {
+        final Mac mac = getMac();
+        final ByteBuffer buffer = ByteBuffer.allocate(mac.getMacLength());
+
+        buffer.putLong(0, counter);
 
         try {
-            final byte[] array = this.buffer.array();
+            final byte[] array = buffer.array();
 
-            this.mac.init(key);
-            this.mac.update(array, 0, 8);
-            this.mac.doFinal(array, 0);
+            mac.init(key);
+            mac.update(array, 0, 8);
+            mac.doFinal(array, 0);
         } catch (final ShortBufferException e) {
             // We allocated the buffer to (at least) match the size of the MAC length at construction time, so this
             // should never happen.
             throw new RuntimeException(e);
         }
 
-        final int offset = this.buffer.get(this.buffer.capacity() - 1) & 0x0f;
-        return (this.buffer.getInt(offset) & 0x7fffffff) % this.modDivisor;
+        final int offset = buffer.get(buffer.capacity() - 1) & 0x0f;
+        return (buffer.getInt(offset) & 0x7fffffff) % this.modDivisor;
+    }
+
+    private Mac getMac() {
+        try {
+            // Cloning is generally cheaper than `Mac.getInstance`, but isn't GUARANTEED to be supported.
+            return (Mac) this.prototypeMac.clone();
+        } catch (CloneNotSupportedException e) {
+            try {
+                return Mac.getInstance(this.prototypeMac.getAlgorithm());
+            } catch (final NoSuchAlgorithmException ex) {
+                // This should be impossible; we're getting the algorithm from a Mac that already exists, and so the
+                // algorithm must be supported.
+                throw new RuntimeException(ex);
+            }
+        }
     }
 
     /**
@@ -216,6 +226,6 @@ public int getPasswordLength() {
      * @return the name of the HMAC algorithm used by this generator
      */
     public String getAlgorithm() {
-        return this.mac.getAlgorithm();
+        return this.prototypeMac.getAlgorithm();
     }
 }

src/test/java/com/eatthepath/otp/HmacOneTimePasswordGeneratorTest.java

@@ -27,8 +27,11 @@
 
 import javax.crypto.spec.SecretKeySpec;
 import java.nio.charset.StandardCharsets;
+import java.security.InvalidKeyException;
 import java.security.Key;
 import java.util.Locale;
+import java.util.Random;
+import java.util.concurrent.*;
 import java.util.stream.Stream;
 
 import static org.junit.jupiter.api.Assertions.*;
@@ -40,6 +43,19 @@ public class HmacOneTimePasswordGeneratorTest {
             new SecretKeySpec("12345678901234567890".getBytes(StandardCharsets.US_ASCII),
                     HmacOneTimePasswordGenerator.HOTP_HMAC_ALGORITHM);
 
+    private static final int[] TEST_VECTORS = new int[] {
+            755224,
+            287082,
+            359152,
+            969429,
+            338314,
+            254676,
+            287922,
+            162583,
+            399871,
+            520489
+    };
+
     @Test
     void testHmacOneTimePasswordGeneratorWithShortPasswordLength() {
         assertThrows(IllegalArgumentException.class, () -> new HmacOneTimePasswordGenerator(5));
@@ -84,23 +100,19 @@ void testGenerateOneTimePassword(final int counter, final int expectedOneTimePas
     void testGenerateOneTimePasswordRepeated() throws Exception {
         final HmacOneTimePasswordGenerator hotpGenerator = new HmacOneTimePasswordGenerator();
 
-        assertEquals(755224, hotpGenerator.generateOneTimePassword(HOTP_KEY, 0));
-        assertEquals(287082, hotpGenerator.generateOneTimePassword(HOTP_KEY, 1));
+        for (int counter = 0; counter < TEST_VECTORS.length; counter++) {
+            assertEquals(TEST_VECTORS[counter], hotpGenerator.generateOneTimePassword(HOTP_KEY, counter));
+        }
     }
 
     private static Stream<Arguments> argumentsForTestGenerateOneTimePasswordHotp() {
-        return Stream.of(
-                arguments(0, 755224),
-                arguments(1, 287082),
-                arguments(2, 359152),
-                arguments(3, 969429),
-                arguments(4, 338314),
-                arguments(5, 254676),
-                arguments(6, 287922),
-                arguments(7, 162583),
-                arguments(8, 399871),
-                arguments(9, 520489)
-        );
+        final Stream.Builder<Arguments> streamBuilder = Stream.builder();
+
+        for (int counter = 0; counter < TEST_VECTORS.length; counter++) {
+            streamBuilder.add(Arguments.of(counter, TEST_VECTORS[counter]));
+        }
+
+        return streamBuilder.build();
     }
 
     @ParameterizedTest
@@ -111,18 +123,13 @@ void testGenerateOneTimePasswordString(final int counter, final String expectedO
     }
 
     private static Stream<Arguments> argumentsForTestGenerateOneTimePasswordStringHotp() {
-        return Stream.of(
-                arguments(0, "755224"),
-                arguments(1, "287082"),
-                arguments(2, "359152"),
-                arguments(3, "969429"),
-                arguments(4, "338314"),
-                arguments(5, "254676"),
-                arguments(6, "287922"),
-                arguments(7, "162583"),
-                arguments(8, "399871"),
-                arguments(9, "520489")
-        );
+        final Stream.Builder<Arguments> streamBuilder = Stream.builder();
+
+        for (int counter = 0; counter < TEST_VECTORS.length; counter++) {
+            streamBuilder.add(Arguments.of(counter, String.valueOf(TEST_VECTORS[counter])));
+        }
+
+        return streamBuilder.build();
     }
 
     @ParameterizedTest
@@ -148,4 +155,53 @@ private static Stream<Arguments> argumentsForTestGenerateOneTimePasswordStringLo
                 arguments(9, locale, "५२०४८९")
         );
     }
+
+    @Test
+    void testConcurrent() throws InterruptedException {
+        final int iterations = 10_000;
+        final int threadCount = Runtime.getRuntime().availableProcessors() * 4;
+        final CountDownLatch threadStartLatch = new CountDownLatch(threadCount);
+        final ExecutorService executorService = Executors.newFixedThreadPool(threadCount);
+        @SuppressWarnings("unchecked") final CompletableFuture<Boolean>[] futures = new CompletableFuture[threadCount];
+
+        final HmacOneTimePasswordGenerator hotpGenerator = new HmacOneTimePasswordGenerator();
+
+        for (int thread = 0; thread < threadCount; thread++) {
+            futures[thread] = CompletableFuture.supplyAsync(() -> {
+                final Random random = new Random();
+                boolean allMatched = true;
+
+                threadStartLatch.countDown();
+
+                try {
+                    threadStartLatch.await();
+                } catch (final InterruptedException e) {
+                    throw new RuntimeException(e);
+                }
+
+                for (int i = 0; i < iterations; i++) {
+                    final int counter = random.nextInt(TEST_VECTORS.length);
+
+                    try {
+                        if (hotpGenerator.generateOneTimePassword(HOTP_KEY, counter) != TEST_VECTORS[counter]) {
+                            allMatched = false;
+                        }
+                    } catch (final InvalidKeyException e) {
+                        throw new RuntimeException(e);
+                    }
+                }
+
+                return allMatched;
+            }, executorService);
+        }
+
+        CompletableFuture.allOf(futures).join();
+
+        for (final CompletableFuture<Boolean> future : futures) {
+            assertTrue(future.join());
+        }
+
+        executorService.shutdown();
+        assertTrue(executorService.awaitTermination(1, TimeUnit.SECONDS));
+    }
 }