suppress ExecutionContext by default in TestServer #10094
Conversation
|
This breaks our recipe for testing things that depend on localized resources in .NET. |
I'm mildly concerned that no tests seem to have failed despite this. Are there tests in this repo that should be catching this break? As per our offline discussion, I'll capture and transfer the culture across. |
| { | ||
| // This will configure IHttpContextAccessor so it needs to happen INSIDE this function, | ||
| // since we are now inside the Server's ExecutionContext. If it happens out |
There was a problem hiding this comment.
Note to self: Don't forget to
|
🆙 📅 now with more culture! |
| /// <summary> | ||
| /// Gets or sets a value that controls if <see cref="ExecutionContext"/> and <see cref="AsyncLocal{T}"/> values are preserved from the client to the server. | ||
| /// </summary> | ||
| public bool PreserveExecutionContext { get; set; } = false; |
There was a problem hiding this comment.
nit: init to false ins't needed here or on line 86.
| public bool AllowSynchronousIO { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets a value that controls if <see cref="ExecutionContext"/> and <see cref="AsyncLocal{T}"/> values are preserved from the client to the server. |
There was a problem hiding this comment.
✔️ Also updating the other comment to move default value to <summary> since <remarks> don't show in IntelliSense (which is where you really want this info IMO).
| @@ -59,15 +60,18 @@ internal void Configure(Action<HttpContext> configureContext) | |||
| /// Start processing the request. | |||
| /// </summary> | |||
| /// <returns></returns> | |||
| internal Task<HttpContext> SendAsync(CancellationToken cancellationToken) | |||
| internal Task<HttpContext> SendAsync(CancellationToken cancellationToken, bool preserveExecutionContext) | |||
There was a problem hiding this comment.
preserveExecutionContext should be a property like AllowSynchronousIO.
There was a problem hiding this comment.
I disagree. It's only used in this method and all the callers have a value they can provide here. AllowSynchronousIO is only a property because HttpContextBuilder implements IHttpBodyControlFeature and requires that the property be provided. In fact, no caller uses the setter on HttpContextBuilder, they only use the constructor argument.
There was a problem hiding this comment.
And why not make it a constructor argument?
fixes #7975 There is a 'PreserveExecutionContext' property to turn the old behavior back on. Also I had to modify where IHttpApplication.CreateContext is called since that's what sets the IHttpContextAccessor, which depends on AsyncLocals!
analogrelay
force-pushed
the
anurse/7975-testserver-suppress-ec
branch
from
6c95f52 to
29b7a64
Compare
|
This comment was made automatically. If there is a problem contact [email protected]. I've triaged the above build. I've created/commented on the following issue(s) |
ghost
deleted the
|
I'm having the same/similar issue but i don't think it's with TestServer. I have a .netstandard 2.0 class library which I have an IHttpContextAccessor. Reading and writing to the session works fine. Then i call a SOAP service through Microsoft.VisualStudio.ConnectedService.Wcf in the class library the session gets (mostly) wiped out. Any key set before calling the webservice persists but the values are wiped out. I'm including the lib in the .netcore webapp Using 2.2 any advice would be greatly appreciated! |
|
@Amd3202 If you're having an issue or have a question I'd suggest filing a new issue and including a sample project that reproduces the issue. Otherwise it's likely we'll lose track of this thread since this is a merged PR. Open issues will show up in our triage process and we'll be able to track it better. |
|
Thanks for the quick reply, i thought it was related... haven't opened a thread on here before i'll see if i can figure that out! Thanks. |
|
Even if it is related, it's easier for us to track in a new open issue rather than as a comment on a closed issue or merged PR :) |
amcasey
added
area-networking
fixes #7975
Currently, the
ExecutionContextflows from client to server inTestServer. This seems odd because the server should be a separate instance. Also, it causes problems when you are running two different apps throughTestServer(such as when you are testing IdentityServer and such). This change changesTestServer's client logic to force a newExecutionContextwhen issuing HTTP requests.There is a
PreserveExecutionContextproperty to turn the old behavior back on. Also I had to modify whereIHttpApplication.CreateContextis called since that's what sets theIHttpContextAccessor. If it's set before we dispatch to the thread pool (thus dropping the EC) the accessor value will be lost when we abandon the EC!I'm reliably informed that there is an MVC test or two depend on the AsyncLocal flow, so I'll fix those in this PR.