Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[API Proposal]: Expose ThreadPool.NotifyThreadBlocked() and NotifyThreadUnblocked() #111102

Open
mriehm opened this issue Jan 6, 2025 · 2 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Threading untriaged New issue has not been triaged by the area owner

Comments

@mriehm
Copy link

mriehm commented Jan 6, 2025

Background and motivation

The ThreadPool can be inappropriately starved of threads if many ThreadPool threads block. The ideal solution to that problem is for applications to write async code to avoid blocking threads. However, sometimes it is impossible for applications to write async code because of a requirement to call a sync API that has no async equivalent.

The particular scenario that I am dealing with is that my ASP.NET app frequently opens handles to files on network storage. Sometimes, the network storage appliance does not open file handles in a timely manner, and so my thread pool threads become blocked for non-trivial amounts of time, which causes the ThreadPool to starve, which causes other requests to not be served in a timely manner. There is no way to open a file handle asynchronously, so I cannot solve this problem by switching to async code.

In .NET 6, PR #53471 modified Task.Wait() to notify the ThreadPool that it is blocking a thread, which informs the ThreadPool to inject new threads more aggressively, My goal with this proposal is to enable application developers to similarly notify the ThreadPool of a thread blocking for reasons other than waiting on a Task.

API Proposal

namespace System.Threading;

public static partial class ThreadPool
{
    public static void NotifyThreadBlocked();

    public static void NotifyThreadUnblocked();
}

These two methods are modeled after the existing internal methods of the same name. The only difference is that the existing internal NotifyThreadBlocked() returns bool to indicate whether to call NotifyThreadUnblocked(), but this proposal has NotifyThreadBlocked() returning void.
I made this change because it simplifies usage, and I assume the only impact to be a negligible performance difference.

I propose that NotifyThreadBlocked() increments a thread local when called, and NotifyThreadUnblocked() decrements that thread local when called, and the thread is only considered unblocked when the thread local is decremented to zero. If NotifyThreadUnblocked() is called when the thread local is already zero, then InvalidOperationException would be thrown.

API Usage

public SafeFileHandle OpenNetworkFile(string networkFilePath)
{
    ThreadPool.NotifyThreadBlocked();
    try
    {
        return NativeMethods.CreateFileW(networkFilePath, ...);
    }
    finally
    {
        ThreadPool.NotifyThreadUnblocked();
    }
}

This example uses a call to the Win32 API CreateFileW() to open a file handle instead of using File.OpenHandle() to open a file handle. I wrote the example this way to sidestep any discussion about whether the problem is better solved by having File.OpenHandle() internally call the existing internal ThreadPool APIs. For the record, I think that File.OpenHandle() probably shouldn't call them, since most files opened by .NET applications in general are not in network storage, so making every caller pay the overhead of notifying the thread pool may not be worth it. Regardless, my ASP.NET app uses native code to open the handle to the network file, so modifying File.OpenHandle() to call the existing internal ThreadPool APIs would not solve my problem.

Alternative Designs

  1. As discussed above, relevant .NET runtime APIs such as File.OpenHandle() or Thread.Sleep() could be modified to call the existing internal ThreadPool APIs instead of exposing the ThreadPool APIs publicly for applications to call. However, this does not solve my case of opening a file handle via native code unless LibraryImportAttribute is enhanced to support calling these APIs, which I'm guessing would an undesirable entanglement of layers. Also, it's not clear that the .NET runtime can always determine when its APIs, such as File.OpenHandle(), are sufficiently likely to block to justify notifying the ThreadPool that the thread is potentially blocking.

  2. Instead of two separate APIs that must be called as a pair, ThreadPool.NotifyThreadBlocked() could return a new ref struct type (e.g. ThreadBlockedScope) that calls an internal ThreadPool.NotifyThreadUnblocked() API in its Dispose(), similar to the recent Lock.Scope ref struct type. This would improve usability by allowing a using-block instead of a try-finally, and it would mitigate potential misuse of forgetting to call NotifyThreadUnblocked(), or calling it the incorrect number of times, or calling it from the wrong thread. The only reason that I didn't make this the initial proposal is because I expect that introducing a new type for this rare and advanced use case may elicit more resistance due to complexity as compared to exposing methods on an existing public type on which the methods are already internally defined. However, I'm happy to switch the proposal to this approach if requested.

  3. Instead of depending on apps to manually indicate with these new ThreadPool APIs that a thread is about to block, perhaps there is some way to solve thread exhaustion with changes internal to the ThreadPool thread injection algorithm, such as with automated detection of blocked threads. I don't have any practical suggestion about how this could be accomplished, though.

  4. For my particular concern about opening files always being synchronous, perhaps it is possible to open files asynchronously on Linux with io_uring. My ASP.NET app runs on Windows, though, where I believe asynchronous file opens to be impossible. Maybe the Windows team could be contacted to request this feature though :).

Risks

  1. Applications could forget to call ThreadPool.NotifyThreadUnblocked() and so 'leak' threads to the thread pool. Or, if NotifyThreadUnblocked() relies on being called on the same thread as NotifyThreadBlocked(), this requirement could be accidentally violated by applications, such as with an intervening await. Since calls to these APIs should be narrowly scoped around a method call that blocks the thread, and since this is an advanced API, the likelihood of misuse seems low. If desired, though, alternative design 2 listed above could mitigate this.

  2. For some ThreadPool implementations, there may be no useful implementation for these APIs, such as when the native Windows thread pool is used. Therefore, use of these APIs may increase behavioral differences between the same application running under different deployment scenarios, or different versions of .NET as ThreadPool implementations are modified. This does not seem a significant concern because the APIs are only performance hints, not guaranteeing specific behavior. Also, the behavioral differences have already been introduced via Task.Wait() in .NET 6+.

  3. It may be unclear to application developers when it is or isn't beneficial for them to call ThreadPool.NotifyThreadBlocked(). For example, Task.Wait() already calls it internally, so applications do not need to call it in that case, but the only way to know that is to read the .NET runtime source code. This could be mitigated with docs for ThreadPool.NotifyThreadBlocked() that list all the runtime APIs that already call it internally, if that list remains sufficiently small. In any case, the impact of an application adding erroneous additional calls to these APIs is probably not significant.

@mriehm mriehm added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jan 6, 2025
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Jan 6, 2025
Copy link
Contributor

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

@filipnavara
Copy link
Member

(I'll refrain from addressing the big questions.)

For the API shape I think it would be preferrable to go with something more robust like the alternative design 2 with disposable ref struct. The original API is too easy to misuse.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Threading untriaged New issue has not been triaged by the area owner
Projects
None yet
Development

No branches or pull requests

2 participants