[ADDED] Option natsOptions_AllowConcurrentTLSHandshakes

src/conn.c

@@ -673,11 +673,21 @@ _makeTLSConn(natsConnection *nc)
 #if defined(NATS_HAS_TLS)
     natsStatus  s       = NATS_OK;
     SSL         *ssl    = NULL;
+    bool        useLock = false;
 
     // Reset nc->errStr before initiating the handshake...
     nc->errStr[0] = '\0';
 
+    // We will lock by default, unless the option `tlsConcurrentHandshakes` allows
+    // us to perform the handshake concurrently. Regardless, we will perform the
+    // handshake under lock for the very first one for this context. This is because
+    // otherwise we get thread sanitizer reports (looks like there is some lazy
+    // initialization happening in the OpenSSL library for the very first handshake).
+
     natsMutex_Lock(nc->opts->sslCtx->lock);
+    useLock = !nc->opts->tlsConcurrentHandshakes || nc->opts->sslCtx->firstHandshake;
+    if (!useLock)
+        natsMutex_Unlock(nc->opts->sslCtx->lock);
 
     ssl = SSL_new(nc->opts->sslCtx->ctx);
     if (ssl == NULL)
@@ -783,19 +793,16 @@ _makeTLSConn(natsConnection *nc)
     if (s == NATS_OK)
     {
         nc->errStr[0] = '\0';
+        nc->sockCtx.ssl = ssl;
+        if (useLock)
+            nc->opts->sslCtx->firstHandshake = false;
     }
-
-    natsMutex_Unlock(nc->opts->sslCtx->lock);
-
-    if (s != NATS_OK)
-    {
-        if (ssl != NULL)
-            SSL_free(ssl);
-    }
-    else
+    else if (ssl != NULL)
     {
-        nc->sockCtx.ssl = ssl;
+        SSL_free(ssl);
     }
+    if (useLock)
+        natsMutex_Unlock(nc->opts->sslCtx->lock);
 
     return NATS_UPDATE_ERR_STACK(s);
 #else

src/nats.h

@@ -3037,6 +3037,47 @@ natsOptions_SetSecure(natsOptions *opts, bool secure);
 NATS_EXTERN natsStatus
 natsOptions_TLSHandshakeFirst(natsOptions *opts);
 
+/** \brief Allows concurrent TLS handshakes.
+ *
+ * When creating a #natsOptions object and configuring it with SSL
+ * related options, an internal SSL context (`SSL_CTX`) is stored in
+ * the #natsOptions object. If this #natsOptions object is passed to
+ * multiple #natsConnection_Connect calls, the connections share this
+ * same `SSL_CTX` object.
+ *
+ * Since those connections can connect (and reconnect) from different
+ * threads, the SSL handshake process is normally protected by a mutex
+ * that belongs to the shared #natsOptions object. It was historically
+ * done this way because a data race is reported (in some cases) if
+ * not the case, and since there was no warning to users that any
+ * SSL verification callback that would be set would need to be
+ * thread safe.
+ *
+ * If there is no SSL verification callback (or they are known to be
+ * thread safe), this option can be used to let the library invoke
+ * the SSL handshake without the aforementioned mutex. This can improve
+ * performance since now multiple connections can happen concurrently
+ * instead of being serialized.
+ *
+ * \warning The very first SSL handshake will be done under the protection
+ * of the mutex, which means other connections (that share the same
+ * #natsOptions object) will not be able to perform their SSL handshake
+ * until the very first is complete. This is because of OpenSSL internal
+ * initialization that is performed during the first handshake. Once that
+ * is done, other SSL handshakes will be done concurrently.
+ *
+ * \note If each connection has its own configured #natsOptions, then there is
+ * no concurrency issue and the SSL handshakes are already performed concurrently,
+ * and therefore this option has no effect. Of course, if SSL verification
+ * callback is passed to different #natsOptions objects, then the callback
+ * has to be thread safe. Even without this option, the risk of data race
+ * would have already existed.
+ *
+ * @param opts the pointer to the #natsOptions obbject.
+ */
+NATS_EXTERN natsStatus
+natsOptions_AllowConcurrentTLSHandshakes(natsOptions *opts);
+
 /** \brief Loads the trusted CA certificates from a file.
  *
  * Loads the trusted CA certificates from a file.

src/natsp.h

@@ -207,6 +207,7 @@ typedef struct __natsSSLCtx
     char            *certFileName;
     char            *keyFileName;
     natsSSLVerifyCb callback;
+    bool            firstHandshake;
 
 } natsSSLCtx;
 
@@ -246,6 +247,7 @@ struct __natsOptions
     bool                    allowReconnect;
     bool                    secure;
     bool                    tlsHandshakeFirst;
+    bool                    tlsConcurrentHandshakes;
     int                     ioBufSize;
     int                     maxReconnect;
     int64_t                 reconnectWait;

src/opts.c

@@ -285,6 +285,7 @@
         SSL_CTX_set_min_proto_version(ctx->ctx, TLS1_2_VERSION);
         SSL_CTX_set_default_verify_paths(ctx->ctx);
 
+        ctx->firstHandshake = true;
         *newCtx = ctx;
     }
     else if (ctx != NULL)
@@ -381,6 +382,22 @@
     return NATS_UPDATE_ERR_STACK(s);
 }
 
+natsStatus
+natsOptions_AllowConcurrentTLSHandshakes(natsOptions *opts)
+{
+    natsStatus s = NATS_OK;
+
+    LOCK_AND_CHECK_OPTIONS(opts, 0);
+
+    s = _getSSLCtx(opts);
+    if (s == NATS_OK)
+        opts->tlsConcurrentHandshakes = true;
+
+    UNLOCK_OPTS(opts);
+
+    return NATS_UPDATE_ERR_STACK(s);
+}
+
 natsStatus
 natsOptions_LoadCATrustedCertificates(natsOptions *opts, const char *fileName)
 {
@@ -818,6 +835,12 @@
     return nats_setError(NATS_ILLEGAL_STATE, "%s", NO_SSL_ERR);
 }
 
+natsStatus
+natsOptions_AllowConcurrentTLSHandshakes(natsOptions *opts)
+{
+    return nats_setError(NATS_ILLEGAL_STATE, "%s", NO_SSL_ERR);
+}
+
 natsStatus
 natsOptions_LoadCATrustedCertificates(natsOptions *opts, const char *fileName)
 {

test/list_test.txt

@@ -273,6 +273,7 @@ _test(SSLHandshakeFirst)
 _test(SSLHandshakeTimeout)
 _test(SSLLoadCAFromMemory)
 _test(SSLMultithreads)
+_test(SSLPerfConcurrentConnect)
 _test(SSLReconnectWithAuthError)
 _test(SSLServerNameIndication)
 _test(SSLSkipServerVerification)

test/test.c

@@ -21470,7 +21470,7 @@ void test_SSLVerifyDynamic(void)
     IFOK(s, natsOptions_LoadCertificatesChainDynamic(opts,
                                                      "certs/cert-dynamic.pem",
                                                      "certs/key-dynamic.pem"));
-    s = natsConnection_Connect(&nc, opts);
+    IFOK(s, natsConnection_Connect(&nc, opts));
     IFOK(s, natsConnection_SubscribeSync(&sub, nc, "*"));
     IFOK(s, natsConnection_PublishString(nc, "foo", "test"));
     IFOK(s, natsConnection_Flush(nc));
@@ -22554,6 +22554,154 @@ void test_SSLAvailable(void)
 #endif
 }
 
+#if defined(NATS_HAS_TLS)
+static void
+_sslConcurrentConn(void *closure)
+{
+    struct threadArg    *args   = (struct threadArg*) closure;
+    natsStatus          s       = NATS_OK;
+    natsConnection      *nc     = NULL;
+
+    natsMutex_Lock(args->m);
+    while (!args->connected)
+        natsCondition_Wait(args->c, args->m);
+    natsMutex_Unlock(args->m);
+
+    s = natsConnection_Connect(&nc, args->opts);
+    if (s != NATS_OK)
+    {
+        natsMutex_Lock(args->m);
+        if (args->status == NATS_OK)
+        {
+            nats_PrintLastErrorStack(stderr);
+            args->status = s;
+        }
+        natsMutex_Unlock(args->m);
+    }
+    natsConnection_Destroy(nc);
+}
+
+#define SSL_CONCURRENT_CONNS 10
+
+static int
+_sslConcurrentVerifyCb(int preverify_ok, void *pctx)
+{
+    // Access some fields to make sure that we are not triggering data races.
+    X509_STORE_CTX      *ctx                = (X509_STORE_CTX*) pctx;
+    X509                *cert               = X509_STORE_CTX_get_current_cert(ctx);
+    SSL                 *ssl                = X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx());
+    STACK_OF(X509)      *chain              = SSL_get_peer_cert_chain(ssl);
+    char                *issuerName         = NULL;
+    int                 result              = 0;
+
+    issuerName = X509_NAME_oneline(X509_get_issuer_name(cert), NULL, 0);
+    if (strstr(issuerName, "Synadia"))
+        result = 1;
+    else
+        result = preverify_ok;
+
+    if (chain == NULL)
+    {
+        // To silence compiler warning...
+    }
+
+    // Sleep on purpose for 500ms. The test creates SSL_CONCURRENT_CONNS
+    // connections in parallel. If this callback invoked during the SSL
+    // handshake had a lock shared by connections (like we used to), the
+    // test would take more than 0.5*SSL_CONCURRENT_CONNS seconds to complete.
+    nats_Sleep(500);
+
+    OPENSSL_free(issuerName);
+
+    return result;
+}
+#endif
+
+void test_SSLPerfConcurrentConnect(void)
+{
+#if defined(NATS_HAS_TLS)
+    natsStatus          s           = NATS_OK;
+    natsOptions         *opts       = NULL;
+    natsPid             serverPid   = NATS_INVALID_PID;
+    int64_t             start       = 0;
+    int64_t             dur         = 0;
+    natsThread          *t[SSL_CONCURRENT_CONNS];
+    int                 i;
+    struct threadArg    args;
+    int64_t             limit;
+
+    s = _createDefaultThreadArgsForCbTests(&args);
+    if (s == NATS_OK)
+        s = natsOptions_Create(&opts);
+    if (opts == NULL)
+        FAIL("Unable to setup test!");
+
+    serverPid = _startServer("nats://127.0.0.1:4443", "-config tls.conf", true);
+    CHECK_SERVER_STARTED(serverPid);
+
+    test("Create options: ");
+    s = natsOptions_SetURL(opts, "nats://127.0.0.1:4443");
+    IFOK(s, natsOptions_SetSecure(opts, true));
+    // For test purposes, we provide the CA trusted certs
+    IFOK(s, natsOptions_LoadCATrustedCertificates(opts, "certs/ca.pem"));
+    IFOK(s, natsOptions_SetExpectedHostname(opts, "localhost"));
+    IFOK(s, natsOptions_AllowConcurrentTLSHandshakes(opts));
+    IFOK(s, natsOptions_SetSSLVerificationCallback(opts, _sslConcurrentVerifyCb));
+    IFOK(s, natsOptions_SetTimeout(opts, 10000));
+    testCond(s == NATS_OK);
+
+    args.opts   = opts;
+    args.status = NATS_OK;
+
+    test("Start threads creating connections: ");
+    for (i=0; (s == NATS_OK) && (i<SSL_CONCURRENT_CONNS); i++)
+        s = natsThread_Create(&t[i], _sslConcurrentConn, &args);
+    testCond(s == NATS_OK);
+
+    test("Notify threads: ");
+    nats_Sleep(250);
+    natsMutex_Lock(args.m);
+    args.connected = true;
+    start = nats_Now();
+    natsCondition_Broadcast(args.c);
+    natsMutex_Unlock(args.m);
+    testCond(true);
+
+    test("Check time: ");
+    for (i=0; i<SSL_CONCURRENT_CONNS; i++)
+    {
+        if (t[i] == NULL)
+            continue;
+
+        natsThread_Join(t[i]);
+        natsThread_Destroy(t[i]);
+    }
+    dur = nats_Now()-start;
+    // Should be less than 0.5*SSL_CONCURRENT_CONNS (that is 0.5*10=5sec)
+    // but account for when running with valgrind where things are slowed
+    // down quite a bit.
+    if (valgrind)
+        limit = 4000;
+    else
+        limit = 2500;
+    testCond(dur < limit);
+
+    test("Check there was no failure: ");
+    natsMutex_Lock(args.m);
+    s = args.status;
+    natsMutex_Unlock(args.m);
+    testCond(s == NATS_OK);
+
+    natsOptions_Destroy(opts);
+
+    _destroyDefaultThreadArgs(&args);
+    _stopServer(serverPid);
+#else
+    test("Skipped when built with no SSL support: ");
+    testCond(true);
+#endif
+}
+
 static void
 rmtree(const char *path)
 {

test/tls.conf

@@ -10,6 +10,6 @@ tls {
   # Server private key
   key_file:  "certs/server-key.pem"
   # Increase timeout for valgrind tests
-  timeout: 2
+  timeout: 5
 }