From ad6f7ea09359b421c673d0d93f9e8cf2cc0e7cb8 Mon Sep 17 00:00:00 2001
From: Corey Kosak <kosak@kosak.com>
Date: Mon, 2 Sep 2024 00:36:04 -0400
Subject: [PATCH] respond to review feedback

---
 .../factories/CredentialsDialogFactory.cs     | 21 +++++++++++++++----
 .../ExcelAddIn/providers/SessionProvider.cs   | 12 ++++++++++-
 2 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/csharp/ExcelAddIn/factories/CredentialsDialogFactory.cs b/csharp/ExcelAddIn/factories/CredentialsDialogFactory.cs
index 8e1d1a27c58..c748847e852 100644
--- a/csharp/ExcelAddIn/factories/CredentialsDialogFactory.cs
+++ b/csharp/ExcelAddIn/factories/CredentialsDialogFactory.cs
@@ -29,22 +29,35 @@ void OnSetCredentialsButtonClicked() {
     var sharedTestCredentialsCookie = new SimpleAtomicReference<object>(new object());
 
     void TestCredentials(CredentialsBase creds) {
-      // Set new version
+      // Make a unique sentinel object to indicate that this thread should be
+      // the one privileged to provide the system with the answer to the "Test
+      // Credentials" question. If the user doesn't press the button again,
+      // we will go ahead and provide our answer to the system. However, if the
+      // user presses the button again, triggering a new thread, then that
+      // new thread will usurp our privilege and it will be the one to provide
+      // the answer.
       var localLatestTcc = new object();
       sharedTestCredentialsCookie.Value = localLatestTcc;
 
       var state = "OK";
       try {
+        // This operation might take some time.
         var temp = SessionBaseFactory.Create(creds, sm.WorkerThread);
         temp.Dispose();
       } catch (Exception ex) {
         state = ex.Message;
       }
 
-      // true if the button was wasn't pressed again in the meantime
-      if (ReferenceEquals(localLatestTcc, sharedTestCredentialsCookie.Value)) {
-        credentialsDialog!.SetTestResultsBox(state);
+      // If sharedTestCredentialsCookie is still the same, then our privilege
+      // has not been usurped and we can provide our answer to the system.
+      // On the other hand, if it changes, then we will just throw away our work.
+      if (!ReferenceEquals(localLatestTcc, sharedTestCredentialsCookie.Value)) {
+        // Our results are moot. Dispose of them.
+        return;
       }
+
+      // Our results are valid. Keep them and tell everyone about it.
+      credentialsDialog!.SetTestResultsBox(state);
     }
 
     void OnTestCredentialsButtonClicked() {
diff --git a/csharp/ExcelAddIn/providers/SessionProvider.cs b/csharp/ExcelAddIn/providers/SessionProvider.cs
index f2471281b90..11882df8144 100644
--- a/csharp/ExcelAddIn/providers/SessionProvider.cs
+++ b/csharp/ExcelAddIn/providers/SessionProvider.cs
@@ -91,18 +91,28 @@ public void SetCredentials(CredentialsBase credentials) {
   }
 
   void CreateSessionBaseInSeparateThread(CredentialsBase credentials) {
+    // Make a unique sentinel object to indicate that this thread should be
+    // the one privileged to provide the system with the Session corresponding
+    // to the credentials. If SetCredentials isn't called in the meantime,
+    // we will go ahead and provide our answer to the system. However, if
+    // SetCredentials is called again, triggering a new thread, then that
+    // new thread will usurp our privilege and it will be the one to provide
+    // the answer.
     var localLatestCookie = new object();
     _sharedSetCredentialsCookie.Value = localLatestCookie;
 
     StatusOr<SessionBase> result;
     try {
+      // This operation might take some time.
       var sb = SessionBaseFactory.Create(credentials, workerThread);
       result = StatusOr<SessionBase>.OfValue(sb);
     } catch (Exception ex) {
       result = StatusOr<SessionBase>.OfStatus(ex.Message);
     }
 
-    // By the time we get here, some time has passed. Have our results become moot?
+    // If sharedTestCredentialsCookie is still the same, then our privilege
+    // has not been usurped and we can provide our answer to the system.
+    // On the other hand, if it has changed, then we will just throw away our work.
     if (!ReferenceEquals(localLatestCookie, _sharedSetCredentialsCookie.Value)) {
       // Our results are moot. Dispose of them.
       if (result.GetValueOrStatus(out var sb, out _)) {