${aspnet-request}: could write some warnings when request isn't available

[komlachsv]

If don't check this:

context.Session != null

in method AspNetRequestValueLayoutRenderer::Append

if try write to log from Global.asax.cs on start application, we have warning:

Exception in layout renderer: System.Web.HttpException (0x80004005): 
   Request is not available in this context

[304NotModified]

Hey,

can you give some more info? You get an exception? Can you post relevant config and c# code?

I don't understand this issue as AspNetRequestValueLayoutRenderer isn't using Session

[304NotModified]

@komlachsv can you give us some more info?

it seems that checking the property HttpContext.Request will raise the http exception (while properties should never raise exceptions as a MS design guidline).
http://stackoverflow.com/questions/2518057/request-is-not-available-in-this-context

[304NotModified]

We can't prevent this. If throwExceptions is set to false, this exception is only logged to the internallogger.

[vegar]

What other implications will setting throwExceptions to false have?
I'm afraid it will hide some potential miss configurations etc.

Would it be an option to wrap the fetching of Request in a utility method, trapping any HttpException thrown?

[304NotModified]

What other implications will setting throwExceptions to false have?
I'm afraid it will hide some potential miss configurations etc.

I hide all errors, so logging could fail without knowing it. But that's a good case? I don't like crashing applications because logging failed.
Note:
You can set 'throwExceptions=falseandthrowConfigExceptions=true`. see http://nlog-project.org/2016/04/16/nlog-4-3-has-been-released.html

Also you can check the internalLogger.

[Xender]

How about just catching the System.Web.HttpException from the property access?
Especially that there is already a null check in place.

I suppose one would have to give up type inferrence and declare httpRequest variable first, then surround the assignement which does property access in a try...catch block, then proceed normally?
So the only non-trivial part seems to be giving up the type inference, and even this looks straight-forward - is this a problem?

[vegar]

I also feel that to handle this very specific and well defined issue with a try/catch would be the best. Wrapped up in something so the issue becomes hidden in the layout code.

[304NotModified]

How about just catching the System.Web.HttpException from the property access?
Especially that there is already a null check in place.

That is indeed possible and not difficult to implement. But what should be do at the catch? An empty catch is bad, so then we log to the internallogger. Also sometimes we like to rethrow the Exception, so we should check "throwExceptions" option.

And then we have the same code which is already in LayoutRenderer.Render: see source ;)

[Xender]

@304NotModified - In this case, we are talking about catching one particular exception in a well-defined case (as @vegar said), not about handling exceptions in general.

Looking at the null check it seems painfully obvious that catch(System.Web.HttpException) { return; } is most appropriate here.

Again, we are talking about catching the exception only when accessing the property, not silencing System.Web.HttpException or any other exception in any other places.

If you think of accessing HttpContext.Current.Request raising exception instead of just being null when it's unavailable as of a broken design, it should be really a no-brainer that this particular exception in this particular situation should be silenced (or, it should do the same thing as the real null check which is already there does - which is to return early).

[304NotModified]

If you think of accessing HttpContext.Current.Request raising exception instead of just being null when it's unavailable as of a broken design, it should be really a no-brainer that this particular exception in this particular situation should be silenced (or, it should do the same thing as the real null check which is already there does - which is to return early).

There are more reasons why we can have an exception in this case, e.g. in .NET CORE when the session isn't setup. So catch(System.Web.HttpException) { return; } is problematic.

[Xender]

@304NotModified - But the context is just expanding a layout renderer.
What will happen is you silence exception and return early from inside a layout renderer?
It will be substituted with an empty string, which is a sensible thing to do from a logging perspective - it means that this information is unavailable in his context.

Will anything else be affected by silencing this exception on property access?
I don't think it can cause any silent failure of application logic or anything else, because if anything will access the property again, the exception will be raised again.

Or do you have a particular case in mind, when accessing the property from inside a logger would cause an exception relevant to the other parts of application be thrown?
It's hard to talk without a particular case to consider, but I'd place pragmatism over purism here.

[304NotModified]

I also prefer pragmatism but I don't see the benefit of another catch - there's already a catch and you can fully control the behavior.

With a plain return in the catch,
the only difference is ignoring throwExceptions and skipping InternalLogger. Is this really an improvement?

[Xender]

I think there is a subtle difference between catching this particular exception and exceptions from logger in general.

It's that while enabling exceptions from logger in general can be useful to debug some other logger issues (i.e. faults inside other layout renderers), letting this particular exception pass is useless, because it's cause is a well-known result broken design, not some unknown error worth investigating.

While other exceptions or exceptions in other context can mean different things, it seems to me that this exception in this context can only really mean that the request is not available in current execution context (please point me out if I'm wrong with any of these assumptions).

If so, why not just think of it as an equivalent of a if-not-null check?

[vegar]

Let's look at this from a different angle: What would you do if the request was not available in the current context, but the property accessor did not throw an exception - it returned null instead. Would you consider it an error state? Would you log to the internal logger that 'something might be wrong here, cause I wanted to log the current request url, but there are no request available' ?

In my code, I like to differentiate between exceptions that I expect and know how to handle and exceptions that I did not expect. If another place (a different layer in my code or others code) throws exception, and I know why, and I know how to recover, I'll not see it as an exception. I'll see it more as 'control flow' than 'exception'.

So if we look away from .net core, which I have no knowledge of, I see no reason to treat the exception thrown inside of the property getter as an exception. It should have returned null, but it doesn't, but we know that an (http)exception thrown in this getter means only one thing: The request does not exist.

Yes, we can just suppress all exceptions thrown in the logging layer - which as you say might be a good idea in production anyway. Maybe. But to me, that sounds like taking down the security net, just because once in a while some things are gonna fall down which we do not wanna catch in that net. And when it comes to the internal logger - I want to be sure that errors popping up in both internal and external logs are error that actually need attention. As soon as we start logging perfectly valid situations as errors, we'll start ignoring the bells and whistles; The alarm goes off - but who cares? I bet it's just the request object not available again...

NLog is a great framework. I really appreciate the work you all have done. This is a part that did not work well for us, though, and we have chosen to reimplemented the layout renderers that we need - with try/catchs - and moved on.

[Xender]

@vegar - I agree with you, and I also decided to silence this exception on my own - but insteadof re-implementing, I've chosen to extend AspNetRequestValueLayoutRenderer class and override DoAppend with a version that does trial access to HttpContext.Request, and proceeds to calling base class method if it doesn't raise.
Maybe this idea could be useful to you too.

But I'd like to see this handled in the library itself rather than by placing a workaround code in the application.

[304NotModified]

I guess it's two against one ;)

OK, I will fix this or sending a PR would also be nice. I don't like empty catches, so writing to the internal log level on debug looks to me the best.

Please be aware that I didn't ignored this request because of purism. It's difficult to implement exception handling in special cases and still have consistent behaviour. For example, the database target was also catching exception differently in the past, with probably a good reason, and that was an issue which multiple users reported over the years.

[vegar]

I fully agree: It's a difficult thing to get right.
And questioning is not the same as ignoring. Me myself loves to apply 'The 5 Whys' to things, even though I know it can be frustrating for the other part. But it's all about understanding and learning and trying to take the right decision.

If you can wait a couple of days, I'll see if I can push a PR.

When it comes to .net core - is the source for the Request property getter different there? The code already contains different code paths for .net core and '.net classic'. Should the behavior be kept as today for .net core?

[304NotModified]

With .NET CORE almost everything is different, especially with ASP.NET. New types, interfaces, properties, missing features and Dependency injection. And there are a lot of breaking changes between rc1 and rc2 (latter not released, unclear when)

[vegar]

It turns out it's very difficult to write a test that expose the error. Do you have some tips?

I've tried two strategies:

1. Just using NSubstitute to make the mocked context to throw when asked for a request, and
2. Use LogManager and a MemoryTarget to actually log something with the request as part of the layout.

Both 'fails' by not rethrowing the exception. The internal Render method of LayoutRenderer has a catch with a ex.MustBeThrown() which returns false.

How can I recreate the scenario we see in our production applications, where the exception is thrown and halting the application?

A third option would be to inspect the InternalLogger in some way, to ensure the exception thrown in the LayoutRenderer doesn't show up there, but I haven't found a way of accessing that either...

[304NotModified]

A third option would be to inspect the InternalLogger in some way, to ensure the exception thrown in the LayoutRenderer doesn't show up there, but I haven't found a way of accessing that either...

You can set InternalLogger.LogWriter for that?

[304NotModified]

Fixed and released in 4.2.1 :)