Prevent flowing ExecutionContext

[Shazwazza]

Prevent flowing the Execution Context to background threads. Without this, it means values from AsyncLocal and CallContext will flow/leak into background threads which should be avoided because it may inadvertently be capturing some ambient contexts like DbContext, etc... which can have unwanted side affects. In most cases anything stored in AsyncLocal is also not thread safe and if it's a mutable object it means concurrent threads can be modifying it which also may cause unwanted side affects. I don't think there's a use-case for flowing the execution context into a background thread since the execution in these threads should be entirely isolated.

I put together a gist showing all the different ways the AsyncLocal/CallContext values flow to child threads here https://gist.github.com/Shazwazza/c127e8c567ab505d146e54ac802a9945

I think this is the only place where the created threads are actually started but I could be wrong. Calling SuppressFlow is only required where the thread is started.

[Shazwazza]

Just downloaded the source since the build is breaking and can see there's a other places that start threads too. Will update.

[Shazwazza]

Seems there's one test failing: SqlServerTimeoutJobFacts.cs:line 240. Unfortunately I'm unable to get this project to build locally in VS (latest) and have tried dotnet build as per docs. Have run out of time today but will see what I can do this week.

[Frazi1]

Hello @Shazwazza

Are you aware of any broken real world use cases because of this?

In our AspNetCore 3.1 app, we use Hangfire and NLog for logging. The minimum log level is set to Warning. Most of the time, everything is fine. But sometimes sql queries from EF Core that go under Information level like SELECT * FROM ... slip into our log files.

We don't use AsyncLocal anywhere in our code. But we have Hangfire.Console + Hangfire.Console.Extensions packages installed. The latter one uses AsyncLocal to keep track of Hangfire's job PerformContext to make it accessible from the DI.

1. Can my issue be related to the problem you're trying to address here?
2. Won't these changes break Hangfire.Console.Extensions package (and possibly others that already use AsyncLocal)?

[Shazwazza]

Hi,

I submitted this based on recent research I as doing in Umbraco CMS which used to use CallContext and now uses AsyncLocal to store an ambient context. Some things in netcore use AsyncLocal too like HttpContextAccessor, ActionContextAccessor and others.

My reasoning for submitting this is because an AsyncLocal (or CallContext) value should only live within it's own execution context whereas a background thread is it's own isolated context. The docs for Hangfire actually state that the work should be done in it's own execution context (i.e. https://docs.hangfire.io/en/latest/getting-started/index.html) but that is actually not the case because without suppressing the flow, the execution context will flow into these background threads.

This becomes problematic if you happen to have anything that is mutable stored in AsyncLocal or CallContext. The general rule is that you "shouldn't" do that because strange things might happen - like if the execution context if flowed to a background long lived thread. An ugly example of this is if there is an IDisposable object in AsyncLocal which is created on the parent thread, then a background task is created and launched and then it is disposed on the parent thread. Meanwhile, the child thread continues to execute and inadvertently accesses a disposed object.

Depending on what libraries do with AsyncLocal/CallContext and if they use an Ambient context design pattern, then flowing this state to a background thread can be problematic. In the case of Umbraco, if an Umbraco Scope is created and a hangfire task is executed then that Scope (potentially along with Sql connection/transactions) will leak and disposed objects may be accessed.

EFCore appears to use AsyncLocal for a few things as boolean values (non mutable) but those values will still flow into the child tasks created by HangFire without SuppressFlow calls.

I'm unsure about the Hangfire.Console.Extensions package and it's usage of AsyncLocal. I can see it's defined here https://github.com/AnderssonPeter/Hangfire.Console.Extensions/blob/985f0afe08c957a0c14659dc4e51c14391655548/Hangfire.Console.Extensions/HangfireSubscriber.cs#L13 but it just depends on if PerformingContext needs to flow from the main thread to the hangfire thread or if it only needs to be available in one or the other (sorry not familiar with what it does).

[Frazi1]

Thanks for the more detailed explanation.

As for Hangfire.Console.Extensions, they have PerformingContextAccessor class which works similar to HttpContextAccessor.

Every Hangfire's running job has a PerformingContext associated with it. By default, you can reference that PerformingContext if only you make it your job method's parameter like public void SendEmailJob(..., PerformingContext ctx) and have Hangfire inject it when the job is run.

PerformingContextAccessor is a Hangfire filter run before and after job execution. It acquires the job PerformContext and stores it in an AsyncLocal field.

It allows you to get the current job PerformingContext anytime within Hangfire job execution scope without having a PerformingContext job parameter.

[Shazwazza]

Ah I see. In the case of passing the PerformingContext via AsyncLocal, then yes using SuppressFlow() will suppress that value from being available in child threads. I understand that it's probably a convenience way to access the PerformingContextAccessor but seems much safer and reliable to have PerformingContext injected.

Perhaps Hangfire needs some way to specify if the ExecutionContext should be suppressed or not when spawning new threads since there's not really any way to control this outside of Hangfire since that needs to be done in the main thread where the child thread is started.

I don't think suppressing ExecutionContext can be done with Hangfire filters since from what I can tell in code these don't execute specifically before/after the thread is started.

Potentially filters do do supply a way to work around the issue of preventing AsyncLocal/CallContext values from flowing to the child threads but I'm unsure which type of filter that could be since it would need to be a filter that is executing on the child thread but before the job code is executed to provide a way to null out specific AsyncLocal/CallContext that should not be flowed.

[odinserj]

Thank you for the information – I didn't know that execution context is flowing even when starting a new thread calling the Start method. However I'm not sure this PR will change anything practically – all the background threads are started when application is being initialized, and usually no AsyncLocal instances are used there. So usually those background threads are already isolated from request handling logic.

At the same time I understand that theoretically it will be useful to provide such an isolation. But to avoid any difficulties and since this can theoretically produce breaking changes in very rare scenarios, I'd suggest to implement this in the dev branch instead to schedule it to version 1.8.

Also I think it will be useful to implement some state reset method that will be called after each background job is completed.

[Shazwazza]

Hi @odinserj , thanks for the info!

all the background threads are started when application is being initialized, and usually no AsyncLocal instances are used there

aha! that's great, that is a key part to all of this then. It's my first time navigating through this code and didn't have time to step through it or anything so couldn't determine how/when the threads were spawned. So yes then in theory AsynLocal or CalContext could only leak to those threads in unique scenarios where someone is booting hangfire a little different I guess.

Happy to rebase on dev, i can do that next week i think.