Address review comments

Author: roji

src/libraries/System.Transactions.Local/src/System/Transactions/DtcProxyShim/DTCInterfaces/ITransactionResourceAsync.cs

@@ -24,7 +24,7 @@ internal interface ITransactionResourceAsync
     /// </summary>
     /// <param name="fRetaining">Always false.</param>
     /// <param name="grfRM">Values from <see cref="OletxXactRm" />.</param>
-    /// <param name="fWantMoniker">Always false.</param> // TODO
+    /// <param name="fWantMoniker">Always false.</param>
     /// <param name="fSinglePhase">If true, it indicates that the RM is the only resource manager enlisted on the transaction.</param>
     void PrepareRequest(
         [MarshalAs(UnmanagedType.Bool)] bool fRetaining,

src/libraries/System.Transactions.Local/src/System/Transactions/DtcProxyShim/DtcProxyShimFactory.cs

@@ -135,7 +135,7 @@ public void BeginTransaction(
         out Guid transactionIdentifier,
         out TransactionShim transactionShim)
     {
-        var options = GetCachedOptions();
+        ITransactionOptions options = GetCachedOptions();
 
         try
         {
@@ -197,7 +197,7 @@ public void ReceiveTransaction(
         out OletxTransactionIsolationLevel isolationLevel,
         out TransactionShim transactionShim)
     {
-        var receiver = GetCachedReceiver();
+        ITransactionReceiver receiver = GetCachedReceiver();
 
         try
         {
@@ -250,7 +250,7 @@ public void GetNotification(
 
         Monitor.Enter(_notificationLock);
 
-        var entryRemoved = _notifications.TryDequeue(out NotificationShimBase? notification);
+        bool entryRemoved = _notifications.TryDequeue(out NotificationShimBase? notification);
         if (entryRemoved)
         {
             managedIdentifier = notification!.EnlistmentIdentifier;

src/libraries/System.Transactions.Local/src/System/Transactions/DtcProxyShim/EnlistmentNotifyShim.cs

@@ -33,7 +33,7 @@ internal void SetIgnoreSpuriousProxyNotifications()
 
     public void PrepareRequest(bool fRetaining, OletxXactRm grfRM, bool fWantMoniker, bool fSinglePhase)
     {
-        var pEnlistmentAsync = Interlocked.Exchange(ref EnlistmentAsync, null);
+        ITransactionEnlistmentAsync? pEnlistmentAsync = Interlocked.Exchange(ref EnlistmentAsync, null);
 
         if (pEnlistmentAsync is null)
         {

src/libraries/System.Transactions.Local/src/System/Transactions/DtcProxyShim/Guids.cs

@@ -6,7 +6,6 @@
 
 namespace System.Transactions.DtcProxyShim;
 
-// TODO: Is this the right to manage these COM IIDs?
 internal static class Guids
 {
     internal const string IID_ITransactionDispenser = "3A6AD9E1-23B9-11cf-AD60-00AA00A74CCD";

src/libraries/System.Transactions.Local/src/System/Transactions/DtcProxyShim/OletxHelper.cs

@@ -34,7 +34,7 @@ internal static void Retry(Action action)
     {
         int nRetries = MaxRetryCount;
 
-        while (nRetries > 0)
+        while (true)
         {
             try
             {
@@ -43,8 +43,12 @@ internal static void Retry(Action action)
             }
             catch (COMException e) when (e.ErrorCode == XACT_E_ALREADYINPROGRESS)
             {
+                if (--nRetries == 0)
+                {
+                    throw;
+                }
+
                 Thread.Sleep(RetryInterval);
-                nRetries--;
             }
         }
     }

src/libraries/System.Transactions.Local/src/System/Transactions/DtcProxyShim/ResourceManagerShim.cs

@@ -25,7 +25,7 @@ public void Enlist(
         var pEnlistmentNotifyShim = new EnlistmentNotifyShim(_shimFactory, managedIdentifier);
         var pEnlistmentShim = new EnlistmentShim(pEnlistmentNotifyShim);
 
-        var transaction = transactionShim.Transaction;
+        ITransaction transaction = transactionShim.Transaction;
         ResourceManager!.Enlist(transaction, pEnlistmentNotifyShim, out Guid txUow, out OletxTransactionIsolationLevel isoLevel, out ITransactionEnlistmentAsync pEnlistmentAsync);
 
         pEnlistmentNotifyShim.EnlistmentAsync = pEnlistmentAsync;

src/libraries/System.Transactions.Local/src/System/Transactions/DtcProxyShim/TransactionShim.cs

@@ -62,7 +62,7 @@ public void GetITransactionNative(out IDtcTransaction transactionNative)
 
     public unsafe byte[] GetPropagationToken()
     {
-        var transmitter = _shimFactory.GetCachedTransmitter(Transaction);
+        ITransactionTransmitter transmitter = _shimFactory.GetCachedTransmitter(Transaction);
 
         try
         {

src/libraries/System.Transactions.Local/src/System/Transactions/DtcProxyShim/VoterShim.cs

@@ -16,7 +16,7 @@ internal VoterBallotShim(DtcProxyShimFactory shimFactory, VoterNotifyShim notify
 
     public void Vote(bool voteYes)
     {
-        var voteHr = OletxHelper.S_OK;
+        int voteHr = OletxHelper.S_OK;
 
         if (!voteYes)
         {

src/libraries/System.Transactions.Local/src/System/Transactions/NonWindowsUnsupported.cs

@@ -197,7 +197,3 @@ internal static Exception NotSupported()
             => new PlatformNotSupportedException(SR.DistributedNotSupported);
     }
 }
-
-namespace System.Transactions.Diagnostics
-{
-}

src/libraries/System.Transactions.Local/src/System/Transactions/Oletx/OletxDependentTransaction.cs

@@ -43,7 +43,7 @@ public void Complete()
 
         Debug.Assert(Disposed == 0, "OletxTransction object is disposed");
 
-        int localCompleted = Interlocked.CompareExchange(ref _completed, 1, 0);
+        int localCompleted = Interlocked.Exchange(ref _completed, 1);
         if (localCompleted == 1)
         {
             throw TransactionException.CreateTransactionCompletedException(DistributedTxId);

src/libraries/System.Transactions.Local/src/System/Transactions/Oletx/OletxEnlistment.cs

@@ -57,8 +57,6 @@ internal OletxEnlistment(
         OletxTransaction oletxTransaction)
         : base(oletxResourceManager, oletxTransaction)
     {
-        Guid myGuid = Guid.Empty;
-
         // This will get set later by the creator of this object after it
         // has enlisted with the proxy.
         EnlistmentShim = null;
@@ -88,8 +86,6 @@ internal OletxEnlistment(
         OletxResourceManager oletxResourceManager)
         : base(oletxResourceManager, null)
     {
-        Guid myGuid = Guid.Empty;
-
         // This will get set later by the creator of this object after it
         // has enlisted with the proxy.
         EnlistmentShim = null;
@@ -304,7 +300,6 @@ public bool PrepareRequest(bool singlePhase, byte[] prepareInfo)
             {
                 State = OletxEnlistmentState.Preparing;
 
-                // TODO: Can this be more efficient.
                 _prepareInfoByteArray = TransactionManager.GetRecoveryInformation(
                     OletxResourceManager.OletxTransactionManager.CreationNodeName,
                     prepareInfo);
@@ -598,7 +593,7 @@ public void Phase0Request(bool abortingHint)
                 // If we got an abort hint or we are the committable transaction and Commit has not yet been called or the TM went down,
                 // we don't want to do any more work on the transaction.  The abort notifications will be sent by the phase 1
                 // enlistment
-                if (_aborting ||  commitNotYetCalled || _tmWentDown)
+                if (_aborting || commitNotYetCalled || _tmWentDown)
                 {
                     // There is a possible race where we could get the Phase0Request before we are given the
                     // shim.  In that case, we will vote "no" when we are given the shim.
@@ -650,7 +645,6 @@ public void Phase0Request(bool abortingHint)
                         rmGuidArray[index];
                 }
 
-                // TODO: Seems like this could be more efficient.
                 _prepareInfoByteArray = TransactionManager.GetRecoveryInformation(
                     OletxResourceManager.OletxTransactionManager.CreationNodeName,
                     _proxyPrepareInfoByteArray);

src/libraries/System.Transactions.Local/src/System/Transactions/Oletx/OletxResourceManager.cs

@@ -571,7 +571,6 @@ internal void ReenlistThread(object? state)
                             localEnlistment = resourceManager.ReenlistList[0] as OletxEnlistment;
                             if (localEnlistment == null)
                             {
-                                //TODO need resource string for this exception.
                                 if (etwLog.IsEnabled())
                                 {
                                     etwLog.InternalError();

src/libraries/System.Transactions.Local/src/System/Transactions/Oletx/OletxTransaction.cs

@@ -119,7 +119,6 @@ internal OletxTransaction(RealOletxTransaction realOletxTransaction)
             RealOletxTransaction.OletxTransactionCreated();
         }
 
-        // TODO: Do we want to keep supporting this?
         protected OletxTransaction(SerializationInfo? serializationInfo, StreamingContext context)
         {
             if (serializationInfo == null)
@@ -711,7 +710,6 @@ internal OletxVolatileEnlistmentContainer AddDependentClone(bool delayCommit)
 
                             if (localPhase0VolatileContainer != null)
                             {
-                                //CSDMain 91509 - We now synchronize this call with the shim notification trying to call Phase0Request on this container
                                 TakeContainerLock(localPhase0VolatileContainer, ref phase0ContainerLockAcquired);
                             }
 
@@ -834,10 +832,7 @@ private static void TakeContainerLock(OletxPhase0VolatileEnlistmentContainer loc
         {
             if (!phase0ContainerLockAcquired)
             {
-#pragma warning disable 0618
-                //@TODO: This overload of Monitor.Enter is obsolete.  Please change this to use Monitor.Enter(ref bool), and remove the pragmas   -- ericeil
                 Monitor.Enter(localPhase0VolatileContainer);
-#pragma warning restore 0618
                 phase0ContainerLockAcquired = true;
             }
         }

src/libraries/System.Transactions.Local/src/System/Transactions/Oletx/OletxTransactionManager.cs

@@ -105,10 +105,7 @@ internal static void ShimNotificationCallback(object? state, bool timeout)
                                 // In this case we know that the TM has gone down and we need to exchange
                                 // the native lock for a managed lock.
                                 ProcessingTmDown = true;
-#pragma warning disable 0618
-                                //@TODO: This overload of Monitor.Enter is obsolete.  Please change this to use Monitor.Enter(ref bool), and remove the pragmas   -- ericeil
                                 Monitor.Enter(ProxyShimFactory);
-#pragma warning restore 0618
                             }
                             else
                             {
@@ -142,7 +139,6 @@ internal static void ShimNotificationCallback(object? state, bool timeout)
                                 {
                                     if (enlistment2 is OletxPhase0VolatileEnlistmentContainer ph0VolEnlistContainer)
                                     {
-                                        //CSDMain 91509 - We now synchronize this call with the AddDependentClone call in RealOleTxTransaction
                                         ph0VolEnlistContainer.Phase0Request(abortingHint);
                                     }
                                     else
@@ -443,7 +439,6 @@ internal OletxCommittableTransaction CreateTransaction(TransactionOptions proper
         DtcTransactionManagerLock.AcquireReaderLock(-1);
         try
         {
-            // TODO: Make Sys.Tx isolation level values the same as DTC isolation level values and use the sys.tx value here.
             OletxTransactionIsolationLevel oletxIsoLevel = ConvertIsolationLevel(properties.IsolationLevel);
             uint oletxTimeout = DtcTransactionManager.AdjustTimeout(properties.Timeout);
 

src/libraries/System.Transactions.Local/src/System/Transactions/Oletx/OletxVolatileEnlistment.cs

@@ -469,7 +469,6 @@ internal void Phase0Request(bool abortHint)
                         try
                         {
                             _phase0EnlistmentShim.Phase0Done(false);
-                            // CSDMain 138031: There is a potential race between DTC sending Abort notification and OletxDependentTransaction::Complete is called.
                             // We need to set the alreadyVoted flag to true once we successfully voted, so later we don't vote again when OletxDependentTransaction::Complete is called
                             // Otherwise, in OletxPhase0VolatileEnlistmentContainer::DecrementOutstandingNotifications code path, we are going to call Phase0Done( true ) again
                             // and result in an access violation while accessing the pPhase0EnlistmentAsync member variable of the Phase0Shim object.
@@ -592,7 +591,7 @@ internal override void AddDependentClone()
         {
             if (Phase != -1)
             {
-                throw TransactionException.CreateTransactionStateException(null, Guid.Empty); // TODO
+                throw TransactionException.CreateTransactionStateException(null, Guid.Empty);
             }
 
             // We simply need to block the response to the proxy until all clone is completed.
@@ -614,8 +613,7 @@ internal override void DependentCloneCompleted()
             etwLog.MethodEnter(TraceSourceType.TraceSourceOleTx, this, description);
         }
 
-        //Fix for stress bug CSDMain 126887. This is to synchronize with the corresponding AddDependentClone
-        //which takes the container lock while incrementing the incompleteDependentClone count
+        // This is to synchronize with the corresponding AddDependentClone which takes the container lock while incrementing the incompleteDependentClone count
         lock (this)
         {
             IncompleteDependentClones--;

src/libraries/System.Transactions.Local/src/System/Transactions/TransactionInterop.cs

@@ -78,10 +78,7 @@ public static byte[] GetExportCookie(Transaction transaction, byte[] whereabouts
                 // (since it is generated by Microsoft code) and the problem is with
                 // communication.  So in this case we default for unknown exceptions to
                 // assume that the problem is with communication.
-
-                // TODO
-                throw new Exception("TODO");
-                // throw TransactionManagerCommunicationException.Create(SR.GetString(SR.TraceSourceOletx), comException);
+                throw TransactionManagerCommunicationException.Create(null, comException);
             }
 
             if (etwLog.IsEnabled())