Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.Sign up
Reset certain thread-local state after thread pool work item execution #6371
It can sometimes happen by accident that a thread pool work item sets some thread-local state such as:
Failing to reset that state causes non-deterministic "leaks" that are very hard to diagnose. This is especially relevant in ASP.NET and in unit testing scenarios. The thread pool is a global process resource. Generally, code should play nice with such resources but bugs should not be catastrophic.
The .NET thread pool should reset all of these items (where practical) after the execution of each work item. I see no valid scenario why someone might want to set these values and have them persist because there is no way to make that deterministic.
Regarding the above items I could not find documentation that specifies which one of them are reset or are not reset. This behavior should be documented and contractually guaranteed.
The behavior changes proposed here should work for the legacy thread pool API as well as for the TPL.
I have not added the execution context and logical call context to the list because I lack the technical understanding to recommend anything about them. Maybe they should be reset as well.
I have not added
The change should not cause much breakage. If someone had relied on this his code only spuriously had the behavior that we are trying to remove here. His code saw mixed behavior. Sometimes the state leaks, sometimes it's clean.
Therefore, his app must be able to tolerate thread local state disappearing at any time. If it now always disappears (instead of sometimes) that should be fine.
The only case this would cause breakage is if there was some very deterministic thread scheduling for some reason that made the thread local state deterministic in practice. Such a thing is hard to imagine. Certainly next to impossible in a web app where requests arrive randomly.
I consider resetting the state as proposed quite a high value change.
Further, should we maybe add an API that allows anyone to reset thread state? That would be useful for custom
The work tasks perform is not expected to rely on thread state. They could potentially use thread locals for caching purposes since a thread pool is expected to reuse threads, and that would be valid. But aside from that, they are not expected to modify thread state that could otherwise negatively affect other tasks. If a task is doing such a thing, to me that would seem to be the problem.
If a task needed to modify thread state for its own benefit, I would expect that it revert any changes it made before it completes, if such a change could negatively affect other tasks or the system as a whole. If this happens accidentally, I would consider that a bug.
Could you please elaborate on that? I would like to see a valid scenario for this.
@kouvel I did not mean to suggest that not resetting the state in the application code is a good thing. I agree it should be considered a bug. But what if such a bug happens? It's catastrophic:
For that reason .NET apps become more stable if the thread pool always resets threads.
Flipping this around: Assume that pool threads were resetting their state today. Would we support a change for them to not reset their state? Of course not, that's absurd because this is clearly worse.
I don't understand what you mean. I do not propose to not reset pool state. In my post I warn of doing that and I advocate systematically resetting. Maybe we misunderstood one another.
There are ways to diagnose these types of problems, they're not impossible to diagnose. I don't think hiding the problem is a good solution.
State changes could be made to be fairly deterministic, for instance if the change is common to all threads. Resetting them could cause perf issues in the app, which could be considered a break. Although the thread-specific state would disappear, the user may have global state that indicates that the current thread should already have the state changes, which could lead to functional problems. I think this has to be considered a breaking change.
Certainly could consider such a change in the interest of performance. Work items are often extremely short and any addition to that path could cause a noticeable degradation in throughput depending on how much resetting is being done. But I would initially choose to not reset state mainly because it places an arbitrary restriction without a good reason for doing so, while there are some good reasons for not doing so.
Something like this may be good for diagnosis. Maybe a flags enum parameter to indicate which states to reset. It would be nice to see what sort of problems are happening and how prevalent these problems are, for considering this API.
We currently use ThreadLocal values in thread pool threads to store values that, for performance, we prefer not to manage statically with locking. E.g., the Random class is not thread safe, but a unique instance stored in ThreadLocal effectively is. Sure we code things to tolerate it disappearing from ThreadLocal, but that defeats the performance goal.
(And please don't suggest that I use a random number generator classes which is thread safe; that's not the point of my example. Random is simply a well-known, non-thread-safe class I can use as an example.)