Web API integration: check for null argument #230
Conversation
lucaspimentel
added
type:bug
area:automatic-instrumentation
| var cancellationToken = ((CancellationTokenSource)cancellationTokenSource).Token; | ||
| if (apiController == null) { throw new ArgumentNullException(nameof(apiController)); } | ||
|
|
||
| if (controllerContext == null) { throw new ArgumentNullException(nameof(controllerContext)); } |
There was a problem hiding this comment.
I'd be in favor of not throwing exceptions to make sure we interfere as little as possible with the normal program execution. Maybe we could log something instead.
There was a problem hiding this comment.
Those arguments can't actually be null in normal program execution. ExecuteAsync() is an instance method and apiController is the this argument.
I removed the null check for controllerContext, since we only use it to populate the span (we already avoid using it if it is null). If it is null, Web API will throw quickly later on.
I left the check for apiController since we actually need that value and we cannot move forward without it. Also, ArgumentNullException looks better and provides more information than the inevitable NullReferenceException.
We got a report of a
NullReferenceExceptioninAspNetWebApi2Integration.ExecuteAsync(). This PR adds a null check for argumentcancellationTokenSourcewith a fallback toCancellationToken.None. Also, if argumentapiControlleris null (should never ever happen), throwArgumentNullExceptionwith the name of the argument to make troubleshooting easier.