Attempt to make it easier to detect when the request is done #1021
Conversation
- Today the async local reference to the HttpContext flows when the execution context is captured. When the http request has ended, the HttpContext property will return the reference to an invalid HttpContext instead of returning null. This change stores both the request id and the HttpContext and makes sure both match before returning anything valid. - This is still racy but should catch more cases of people doing bad things. - There will still be issue if people store the context in a local and use that reference instead of accessing it through the property getter but we can live with that. - Added tests
| else | ||
| { | ||
| // Setting the context to null means the request is over | ||
| existing.context.TraceIdentifier = null; |
There was a problem hiding this comment.
Don't modify the actual context, let kestrel clean that up.
There was a problem hiding this comment.
Is set to null in HttpProtocol.Reset()?
There was a problem hiding this comment.
WebListener doesn't do this today so I'm not sure I want to remove it yet. It means all server implementations need to mutate the HttpContext on Dispose.
There was a problem hiding this comment.
That's better than having an arbitrary field mutated by the accessor. Any way to give it its own field? A feature?
There was a problem hiding this comment.
I originally had that but it will make the performance worse (as it modifies the feature. We already have a feature that I'm piggy backing on. I would need to make a new feature and implement it in all the server feature collections then clear it here. It's exactly the same as this.
So the question is, what's the downside?
- Set the TraceIdentifier to null in the default HttpContextFactory - Added more tests
| } | ||
|
|
||
| [Fact] | ||
| public async Task HttpContextAccessor_GettingHttpContextReturnsNullHttpContextIsDifferentTraceIdentifier() |
|
|
||
| public HttpContext HttpContext | ||
| { | ||
| get | ||
| { | ||
| return _httpContextCurrent.Value; | ||
| var value = _httpContextCurrent.Value; | ||
| // Only return the context if the stored request id matches the stored trace identifier |
There was a problem hiding this comment.
// TraceIdentifier is cleared by HttpContextFactory.Dispose.
| } | ||
| set | ||
| { | ||
| _httpContextCurrent.Value = value; | ||
| _httpContextCurrent.Value = (value?.TraceIdentifier, value); |
There was a problem hiding this comment.
Is there ever a situation where a non-null HttpContext is set with a null TraceIdentifier? If not, this could be simplified quite a bit.
There was a problem hiding this comment.
Nvm. I don't think it can be easily simplified.
Mitigates aspnet/KestrelHttpServer#2591
cc @benaadams