Repository: stripe/stripe-node
Pull request #1205 (2021-07-28 18:29:32)

Prevent fetching the user agent concurrently
r? @dcr-stripe 

## Motivation
Fixes https://github.com/stripe/stripe-node/issues/1202

## Summary
Right now, when calculating the proper user agent, we check a cache and shell out to `uname` if the cache is empty. This is bad because we can shell out to `uname` multiple times concurrently before the cache is populated. For some bulk operation usage patterns this causes an unwanted CPU spike (see #1202). It would be better to only shell out once.

This PR makes two changes:

1. Makes the cache use a promise. Promises are very good at resolving a value only once.
2. Caches the result of `uname` directly, instead of the derived `user_agent`. This allows me to remove some questionable cache invalidation. We should only ever need to call `uname` once, `uname` is not expected to change.

**Breakingness:**

Stripe.USER_AGENT_SERIALIZED isn't marked as private, but I think we should consider it safe to just remove this property. It was an internal cache and I cannot imagine any legitimate use case relying on it.
## Tests
Existing tests for `getClientUserAgent` pass.


Patch:
diff --git a/lib/stripe.js b/lib/stripe.js
index 814c7fea..ab83c379 100644
--- a/lib/stripe.js
+++ b/lib/stripe.js
@@ -21,7 +21,8 @@ Stripe.USER_AGENT = {
   typescript: false,
 };
 
-Stripe.USER_AGENT_SERIALIZED = null;
+/** @private */
+Stripe._UNAME_CACHE = null;
 
 const MAX_NETWORK_RETRY_DELAY_SEC = 2;
 const INITIAL_NETWORK_RETRY_DELAY_SEC = 0.5;
@@ -102,7 +103,6 @@ function Stripe(key, config = {}) {
     // serializing the user agent involves shelling out to the system,
     // and given some users may instantiate the library many times without switching between TS and non-TS,
     // we only want to incur the performance hit when that actually happens.
-    Stripe.USER_AGENT_SERIALIZED = null;
     Stripe.USER_AGENT.typescript = typescript;
   }
 
@@ -286,9 +286,6 @@ Stripe.prototype = {
       return accum;
     }, undefined);
 
-    // Kill the cached UA string because it may no longer be valid
-    Stripe.USER_AGENT_SERIALIZED = undefined;
-
     this._appInfo = appInfo;
   },
 
@@ -396,6 +393,20 @@ Stripe.prototype = {
     return INITIAL_NETWORK_RETRY_DELAY_SEC;
   },
 
+  /**
+   * @private
+   */
+  getUname(cb) {
+    if (!Stripe._UNAME_CACHE) {
+      Stripe._UNAME_CACHE = new Promise((resolve) => {
+        utils.safeExec('uname -a', (err, uname) => {
+          resolve(uname);
+        });
+      });
+    }
+    Stripe._UNAME_CACHE.then((uname) => cb(uname));
+  },
+
   /**
    * @private
    * Please open or upvote an issue at github.com/stripe/stripe-node
@@ -407,13 +418,7 @@ Stripe.prototype = {
    * speed advantage.
    */
   getClientUserAgent(cb) {
-    if (Stripe.USER_AGENT_SERIALIZED) {
-      return cb(Stripe.USER_AGENT_SERIALIZED);
-    }
-    this.getClientUserAgentSeeded(Stripe.USER_AGENT, (cua) => {
-      Stripe.USER_AGENT_SERIALIZED = cua;
-      cb(Stripe.USER_AGENT_SERIALIZED);
-    });
+    return this.getClientUserAgentSeeded(Stripe.USER_AGENT, cb);
   },
 
   /**
@@ -427,7 +432,7 @@ Stripe.prototype = {
    * fetching a uname from the system.
    */
   getClientUserAgentSeeded(seed, cb) {
-    utils.safeExec('uname -a', (err, uname) => {
+    this.getUname((uname) => {
       const userAgent = {};
       for (const field in seed) {
         userAgent[field] = encodeURIComponent(seed[field]);


Test patch:
diff --git a/test/stripe.spec.js b/test/stripe.spec.js
index 26fcba6b..74027c6e 100644
--- a/test/stripe.spec.js
+++ b/test/stripe.spec.js
@@ -212,6 +212,9 @@ describe('Stripe Module', function() {
 
     describe('uname', () => {
       let origExec;
+      beforeEach(() => {
+        Stripe._UNAME_CACHE = null;
+      });
       beforeEach(() => {
         origExec = utils.safeExec;
       });