Remove Debug.Assert from Http2OutputProducer

[halter73]

#11560 would have likely been much easier to diagnose had we not swallowed the exception using Debug.Assert.

Back when we ran tests agains debug builds, the Debug.Asserts made a little more sense, but now the asserts pretty much do nothing. You'll only ever see assertion failures if you build locally.

Ideally, I'd like to replace all Debug.Asserts in the entire AspNetCore repo to Trace.Assert (which is a lot like Debug.Assert but doesn't get compiled out of release build) or critical logs. Critical logs should fail most of our tests, but might not fail unit tests that effectively use a no-op logger.

We might have to be careful in places where Debug.Assert is called in a tight loop, but even in methods that are called relatively frequently like Http2OutputProducer.ProcessDataWrites can certainly handle checking an extra condition per call.

For now, I'm just fixing Http2OutputProducer to start the discussion and to stop swallowing exceptions we know we want to see.

[halter73]

This assertion never failed as far as I'm aware, but that was the only other Debug.Assert in the class, so I changed it so it also just results in a critical log if it fails.

[benaadams]

Back when we ran tests agains debug builds, the Debug.Asserts made a little more sense, but now the asserts pretty much do nothing. You'll only ever see assertion failures if you build locally.

Maybe should also run a few Debug builds? A Windows & a Linux?

Some of the Debug.Asserts are to confirm assumptions linking methods and ensure they aren't accidentally changed rather than anything to be checked at release time.

[halter73]

Some of the Debug.Asserts are to confirm assumptions linking methods and ensure they aren't accidentally changed rather than anything to be checked at release time.

At first, I looked at removing all Debug.Asserts from the Kestrel solution for this first PR, but I saw the Debug.Asserts you mentioned. While even those asserts aren't in a super tight loop, I also agree that it feels bad doing extra in release builds for things that couldn't possibly go wrong.

The thing is that things that cannot possibly go wrong eventually can go wrong after a big refactoring as demonstrated by #11560. For this, I think a critical log is definitely the right solution. For some of the other asserts, I think just ensuring we have some Debug build test coverage should be sufficient.

The tough part is trying to come up with clear guidance for what should be Debug.Asserted vs what should be critically logged vs what should be Trace.Asserted.

[halter73]

What I don't think is acceptable is leaving everything as Debug.Assert and not having any automated test coverage for debug builds.

[halter73]

I was going to wait for #11601, but all the tests seem to be still passing, and I'd rather see the log than not, so I'm merging this now.

[benaadams]

I believe Debug builds were running on previous CI?

The tough part is trying to come up with clear guidance for what should be Debug.Asserted vs what should be critically logged vs what should be Trace.Asserted.

Don't think Trace.Assert should be used and throwing a regular exception is probably better; so I'd go for Debug.Assert, logged, exception thrown.

Debug.Assert is useful say where an outer method is checking parameters and it calls another method; they you may want to Debug.Assert those pre-checked parameters to both indicate what they should be for refactoring and confirm their state. However, you do need to still be testing a Debug build for them to hold :)

[halter73]

Well Debug.Assert(false, ex.ToString()); wouldn't even work in a Debug build; is a bit strange.

I'm not sure what you mean here. It definitely worked to some extent in a Debug build. That's how I got this output..

Don't think Trace.Assert should be used and throwing a regular exception is probably better;

Throwing a regular exception is quite different from an assertion though. If it's something that should never happen, you don't want to risk something catching and swallowing the exception. You want to always fail fast with an error like Trace/Debug.Assert does. This can sometimes make it difficult to find the test that triggered the fail-fast/assert-failure, but that's better than the assertion being swallowed and ignore.

[benaadams]

I'm not sure what you mean here. ...

I was being a bit slow :)

[benaadams]

Throwing a regular exception is quite different from an assertion though. If it's something that should never happen, you don't want to risk something catching and swallowing the exception. You want to always fail fast with an error like Trace/Debug.Assert does.

If its recoverable at the process level (e.g. reject request, move on to next) a Fail Fast is embedding a potential DoS into the server if it can ever be triggered externally at runtime as continual full process restart leads to a very low request handling rate. If it would lead to or suggest process wide corruption to continue then Fail Fast makes sense.

[halter73]

I completely agree about not wanting to turn an unexpected connection-level error into a DoS vector. We definitely need to be very careful with Trace.Asserts, and should reserve it's usage for assertions that would indicate a process-wide issue if violated.