Delete NetEventSource.Fail

At some point some Debug.Asserts/Fails were replaced by this NetEventSource.Fail helper, which both Debug.Fails and fires an EventSource event.  But asserts in our code base are intended for things that should never happen, and we needn't be emitting events for them (if we did want to emit events for them, we'd need to tackle the other ~20,000 Debug.Assert/Fails in the codebase.

I've deleted NetEventSource.Fail, and fixed up the call sites.  Some were simply replaced by Debug.Assert/Fail.  Some were deleted entirely, when from code inspection it looked like they could actually be hit, but were guarded by a check for the event source being enabled and thus were unlikely to have been triggered in our previous testing.  Etc.

Author: stephentoub

src/libraries/Common/src/Interop/Windows/SspiCli/SSPIAuthType.cs

@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 #nullable enable
+using System.Diagnostics;
 using System.Net.Security;
 using System.Runtime.InteropServices;
 
@@ -93,7 +94,7 @@ public unsafe int DecryptMessage(SafeDeleteContext context, ref Interop.SspiCli.
 
             if (status == 0 && qop == Interop.SspiCli.SECQOP_WRAP_NO_ENCRYPT)
             {
-                NetEventSource.Fail(this, $"Expected qop = 0, returned value = {qop}");
+                Debug.Fail($"Expected qop = 0, returned value = {qop}");
                 throw new InvalidOperationException(SR.net_auth_message_not_encrypted);
             }
 

src/libraries/Common/src/Interop/Windows/SspiCli/SSPIWrapper.cs

@@ -226,6 +226,7 @@ private ref struct ThreeByteArrays
 
         private static unsafe int EncryptDecryptHelper(OP op, ISSPIInterface secModule, SafeDeleteContext context, Span<SecurityBuffer> input, uint sequenceNumber)
         {
+            Debug.Assert(Enum.IsDefined<OP>(op), $"Unknown op: {op}");
             Debug.Assert(input.Length <= 3, "The below logic only works for 3 or fewer buffers.");
 
             Interop.SspiCli.SecBufferDesc sdcInOut = new Interop.SspiCli.SecBufferDesc(input.Length);
@@ -259,29 +260,13 @@ private static unsafe int EncryptDecryptHelper(OP op, ISSPIInterface secModule,
                 }
 
                 // The result is written in the input Buffer passed as type=BufferType.Data.
-                int errorCode;
-                switch (op)
+                int errorCode = op switch
                 {
-                    case OP.Encrypt:
-                        errorCode = secModule.EncryptMessage(context, ref sdcInOut, sequenceNumber);
-                        break;
-
-                    case OP.Decrypt:
-                        errorCode = secModule.DecryptMessage(context, ref sdcInOut, sequenceNumber);
-                        break;
-
-                    case OP.MakeSignature:
-                        errorCode = secModule.MakeSignature(context, ref sdcInOut, sequenceNumber);
-                        break;
-
-                    case OP.VerifySignature:
-                        errorCode = secModule.VerifySignature(context, ref sdcInOut, sequenceNumber);
-                        break;
-
-                    default:
-                        NetEventSource.Fail(null, $"Unknown OP: {op}");
-                        throw NotImplemented.ByDesignWithMessage(SR.net_MethodNotImplementedException);
-                }
+                    OP.Encrypt => secModule.EncryptMessage(context, ref sdcInOut, sequenceNumber),
+                    OP.Decrypt => secModule.DecryptMessage(context, ref sdcInOut, sequenceNumber),
+                    OP.MakeSignature => secModule.MakeSignature(context, ref sdcInOut, sequenceNumber),
+                    _ /* OP.VerifySignature */ => secModule.VerifySignature(context, ref sdcInOut, sequenceNumber),
+                };
 
                 // Marshalling back returned sizes / data.
                 for (int i = 0; i < input.Length; i++)
@@ -320,35 +305,23 @@ private static unsafe int EncryptDecryptHelper(OP op, ISSPIInterface secModule,
 
                         if (j >= input.Length)
                         {
-                            NetEventSource.Fail(null, "Output buffer out of range.");
+                            Debug.Fail("Output buffer out of range.");
                             iBuffer.size = 0;
                             iBuffer.offset = 0;
                             iBuffer.token = null;
                         }
                     }
 
                     // Backup validate the new sizes.
-                    if (iBuffer.offset < 0 || iBuffer.offset > (iBuffer.token == null ? 0 : iBuffer.token.Length))
-                    {
-                        NetEventSource.Fail(null, $"'offset' out of range.  [{iBuffer.offset}]");
-                    }
-
-                    if (iBuffer.size < 0 || iBuffer.size > (iBuffer.token == null ? 0 : iBuffer.token.Length - iBuffer.offset))
-                    {
-                        NetEventSource.Fail(null, $"'size' out of range.  [{iBuffer.size}]");
-                    }
+                    Debug.Assert(iBuffer.offset >= 0 && iBuffer.offset <= (iBuffer.token == null ? 0 : iBuffer.token.Length), $"'offset' out of range.  [{iBuffer.offset}]");
+                    Debug.Assert(iBuffer.size >= 0 && iBuffer.size <= (iBuffer.token == null ? 0 : iBuffer.token.Length - iBuffer.offset), $"'size' out of range.  [{iBuffer.size}]");
                 }
 
                 if (NetEventSource.Log.IsEnabled() && errorCode != 0)
                 {
-                    if (errorCode == Interop.SspiCli.SEC_I_RENEGOTIATE)
-                    {
-                        NetEventSource.Error(null, SR.Format(SR.event_OperationReturnedSomething, op, "SEC_I_RENEGOTIATE"));
-                    }
-                    else
-                    {
-                        NetEventSource.Error(null, SR.Format(SR.net_log_operation_failed_with_error, op, $"0x{0:X}"));
-                    }
+                    NetEventSource.Error(null, errorCode == Interop.SspiCli.SEC_I_RENEGOTIATE ?
+                        SR.Format(SR.event_OperationReturnedSomething, op, "SEC_I_RENEGOTIATE") :
+                        SR.Format(SR.net_log_operation_failed_with_error, op, $"0x{0:X}"));
                 }
 
                 return errorCode;

src/libraries/Common/src/System/Net/ContextAwareResult.cs

@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 #nullable enable
+using System.Diagnostics;
 using System.Threading;
 
 namespace System.Net
@@ -130,11 +131,7 @@ internal ExecutionContext? ContextCopy
             {
                 if (InternalPeekCompleted)
                 {
-                    if ((_flags & StateFlags.ThreadSafeContextCopy) == 0)
-                    {
-                        NetEventSource.Fail(this, "Called on completed result.");
-                    }
-
+                    Debug.Assert((_flags & StateFlags.ThreadSafeContextCopy) != 0, "Called on completed result.");
                     throw new InvalidOperationException(SR.net_completed_result);
                 }
 
@@ -145,29 +142,19 @@ internal ExecutionContext? ContextCopy
                 }
 
                 // Make sure the context was requested.
-                if (AsyncCallback == null && (_flags & StateFlags.CaptureContext) == 0)
-                {
-                    NetEventSource.Fail(this, "No context captured - specify a callback or forceCaptureContext.");
-                }
+                Debug.Assert(AsyncCallback != null || (_flags & StateFlags.CaptureContext) != 0, "No context captured - specify a callback or forceCaptureContext.");
 
                 // Just use the lock to block.  We might be on the thread that owns the lock which is great, it means we
                 // don't need a context anyway.
                 if ((_flags & StateFlags.PostBlockFinished) == 0)
                 {
-                    if (_lock == null)
-                    {
-                        NetEventSource.Fail(this, "Must lock (StartPostingAsyncOp()) { ... FinishPostingAsyncOp(); } when calling ContextCopy (unless it's only called after FinishPostingAsyncOp).");
-                    }
+                    Debug.Assert(_lock != null, "Must lock (StartPostingAsyncOp()) { ... FinishPostingAsyncOp(); } when calling ContextCopy (unless it's only called after FinishPostingAsyncOp).");
                     lock (_lock) { }
                 }
 
                 if (InternalPeekCompleted)
                 {
-                    if ((_flags & StateFlags.ThreadSafeContextCopy) == 0)
-                    {
-                        NetEventSource.Fail(this, "Result became completed during call.");
-                    }
-
+                    Debug.Assert((_flags & StateFlags.ThreadSafeContextCopy) != 0, "Result became completed during call.");
                     throw new InvalidOperationException(SR.net_completed_result);
                 }
 
@@ -197,11 +184,7 @@ internal object StartPostingAsyncOp()
         // object from being created.
         internal object? StartPostingAsyncOp(bool lockCapture)
         {
-            if (InternalPeekCompleted)
-            {
-                NetEventSource.Fail(this, "Called on completed result.");
-            }
-
+            Debug.Assert(!InternalPeekCompleted, "Called on completed result.");
             DebugProtectState(true);
 
             _lock = lockCapture ? new object() : null;
@@ -285,10 +268,7 @@ protected override void Cleanup()
         // Returns whether the operation completed sync or not.
         private bool CaptureOrComplete(ref ExecutionContext? cachedContext, bool returnContext)
         {
-            if ((_flags & StateFlags.PostBlockStarted) == 0)
-            {
-                NetEventSource.Fail(this, "Called without calling StartPostingAsyncOp.");
-            }
+            Debug.Assert((_flags & StateFlags.PostBlockStarted) != 0, "Called without calling StartPostingAsyncOp.");
 
             // See if we're going to need to capture the context.
             bool capturingContext = AsyncCallback != null || (_flags & StateFlags.CaptureContext) != 0;
@@ -334,10 +314,7 @@ private bool CaptureOrComplete(ref ExecutionContext? cachedContext, bool returnC
                 if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, "Skipping capture");
 
                 cachedContext = null;
-                if (AsyncCallback != null && !CompletedSynchronously)
-                {
-                    NetEventSource.Fail(this, "Didn't capture context, but didn't complete synchronously!");
-                }
+                Debug.Assert(AsyncCallback == null || CompletedSynchronously, "Didn't capture context, but didn't complete synchronously!");
             }
 
             // Now we want to see for sure what to do.  We might have just captured the context for no reason.

src/libraries/Common/src/System/Net/Http/aspnetcore/NetEventSource.Common.cs

@@ -35,9 +35,6 @@ namespace System.Net
     //       NetEventSource.Info(this, "literal string");  // arbitrary message with a literal string
     //   Debug.Asserts inside the logging methods will help to flag some misuse if the DEBUG_NETEVENTSOURCE_MISUSE compilation constant is defined.
     //   However, because it can be difficult by observation to understand all of the costs involved, guarding can be done everywhere.
-    // - NetEventSource.Fail calls typically do not need to be prefixed with an IsEnabled check, even if they allocate, as FailMessage
-    //   should only be used in cases similar to Debug.Fail, where they are not expected to happen in retail builds, and thus extra costs
-    //   don't matter.
     // - Messages can be strings, formattable strings, or any other object.  Objects (including those used in formattable strings) have special
     //   formatting applied, controlled by the Format method.  Partial specializations can also override this formatting by implementing a partial
     //   method that takes an object and optionally provides a string representation of it, in case a particular library wants to customize further.
@@ -70,7 +67,6 @@ public class Keywords
         private const int AssociateEventId = 3;
         private const int InfoEventId = 4;
         private const int ErrorEventId = 5;
-        private const int CriticalFailureEventId = 6;
         private const int DumpArrayEventId = 7;
 
         // These events are implemented in NetEventSource.Security.cs.
@@ -253,40 +249,6 @@ private void ErrorMessage(string thisOrContextObject, string? memberName, string
             WriteEvent(ErrorEventId, thisOrContextObject, memberName ?? MissingMember, message);
         #endregion
 
-        #region Fail
-        /// <summary>Logs a fatal error and raises an assert.</summary>
-        /// <param name="thisOrContextObject">`this`, or another object that serves to provide context for the operation.</param>
-        /// <param name="formattableString">The message to be logged.</param>
-        /// <param name="memberName">The calling member.</param>
-        [NonEvent]
-        public static void Fail(object? thisOrContextObject, FormattableString formattableString, [CallerMemberName] string? memberName = null)
-        {
-            // Don't call DebugValidateArg on args, as we expect Fail to be used in assert/failure situations
-            // that should never happen in production, and thus we don't care about extra costs.
-
-            if (IsEnabled) Log.CriticalFailure(IdOf(thisOrContextObject), memberName, Format(formattableString));
-            Debug.Fail(Format(formattableString), $"{IdOf(thisOrContextObject)}.{memberName}");
-        }
-
-        /// <summary>Logs a fatal error and raises an assert.</summary>
-        /// <param name="thisOrContextObject">`this`, or another object that serves to provide context for the operation.</param>
-        /// <param name="message">The message to be logged.</param>
-        /// <param name="memberName">The calling member.</param>
-        [NonEvent]
-        public static void Fail(object? thisOrContextObject, object message, [CallerMemberName] string? memberName = null)
-        {
-            // Don't call DebugValidateArg on args, as we expect Fail to be used in assert/failure situations
-            // that should never happen in production, and thus we don't care about extra costs.
-
-            if (IsEnabled) Log.CriticalFailure(IdOf(thisOrContextObject), memberName, Format(message).ToString());
-            Debug.Fail(Format(message).ToString(), $"{IdOf(thisOrContextObject)}.{memberName}");
-        }
-
-        [Event(CriticalFailureEventId, Level = EventLevel.Critical, Keywords = Keywords.Debug)]
-        private void CriticalFailure(string thisOrContextObject, string? memberName, string? message) =>
-            WriteEvent(CriticalFailureEventId, thisOrContextObject, memberName ?? MissingMember, message);
-        #endregion
-
         #region DumpBuffer
         /// <summary>Logs the contents of a buffer.</summary>
         /// <param name="thisOrContextObject">`this`, or another object that serves to provide context for the operation.</param>
@@ -307,14 +269,8 @@ public static void DumpBuffer(object? thisOrContextObject, byte[] buffer, [Calle
         [NonEvent]
         public static void DumpBuffer(object? thisOrContextObject, byte[] buffer, int offset, int count, [CallerMemberName] string? memberName = null)
         {
-            if (IsEnabled)
+            if (IsEnabled && offset >= 0 && offset <= buffer.Length - count)
             {
-                if (offset < 0 || offset > buffer.Length - count)
-                {
-                    Fail(thisOrContextObject, $"Invalid {nameof(DumpBuffer)} Args. Length={buffer.Length}, Offset={offset}, Count={count}", memberName);
-                    return;
-                }
-
                 count = Math.Min(count, MaxDumpSize);
 
                 byte[] slice = buffer;

src/libraries/Common/src/System/Net/InternalException.cs

@@ -2,24 +2,20 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 #nullable enable
+using System.Diagnostics;
+
 namespace System.Net
 {
     internal sealed class InternalException : Exception
     {
         private readonly object? _unexpectedValue;
 
-        internal InternalException()
-        {
-            NetEventSource.Fail(this, "InternalException thrown.");
-        }
+        internal InternalException() : this(null) { }
 
-        internal InternalException(object unexpectedValue)
+        internal InternalException(object? unexpectedValue)
         {
+            Debug.Fail($"InternalException thrown for unexpected value: {unexpectedValue}");
             _unexpectedValue = unexpectedValue;
-            if (NetEventSource.Log.IsEnabled())
-            {
-                NetEventSource.Fail(this, $"InternalException thrown for unexpected value: {unexpectedValue}");
-            }
         }
 
         public override string Message => _unexpectedValue != null ?

src/libraries/Common/src/System/Net/LazyAsyncResult.cs

@@ -257,15 +257,9 @@ internal object? Result
                 // then the "result" parameter passed to InvokeCallback() will be ignored.
 
                 // It's an error to call after the result has been completed or with DBNull.
-                if (value == DBNull.Value)
-                {
-                    NetEventSource.Fail(this, "Result can't be set to DBNull - it's a special internal value.");
-                }
+                Debug.Assert(value != DBNull.Value, "Result can't be set to DBNull - it's a special internal value.");
 
-                if (InternalPeekCompleted)
-                {
-                    NetEventSource.Fail(this, "Called on completed result.");
-                }
+                Debug.Assert(!InternalPeekCompleted, "Called on completed result.");
                 _result = value;
             }
         }