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

Add Thread.UnsafeStart to avoid capturing the ExecutionContext #46594

Closed
davidfowl opened this issue Jan 5, 2021 · 4 comments · Fixed by #47056
Closed

Add Thread.UnsafeStart to avoid capturing the ExecutionContext #46594

davidfowl opened this issue Jan 5, 2021 · 4 comments · Fixed by #47056
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Threading
Milestone

Comments

@davidfowl
Copy link
Member

davidfowl commented Jan 5, 2021

Background and Motivation

We added UnsafeStart in this PR #46181 because we needed to lazily create thread pool threads and the timer thread on the default execution context. UnsafeStart avoids capturing the current execution context and restoring it when the thread runs. There are other places where we create threads that could use similar logic:

These are places that should start on the default execution context.

Proposed API

namespace System.Threading
{
     public class Thread 
     {
+       void UnsafeStart();
+       void UnsafeStart(object? parameter);
     }

Usage Examples

var local = new AsyncLocal<int>();
local.Value = 10;
var thread= new Thead(() => 
{
    var value = local.Value;
    while (true)
    {
         Console.WriteLine(value++);
         Thread.Sleep(5000);
    }
});

thread.UnsafeStart();

Alternative Designs

Passing a boolean to the thread constructor but that seemed like a bad idea since Start is the thing capturing the execution.

Risks

None.

@davidfowl davidfowl added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jan 5, 2021
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Threading untriaged New issue has not been triaged by the area owner labels Jan 5, 2021
@stephentoub
Copy link
Member

LGTM as proposed

@stephentoub stephentoub added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner labels Jan 5, 2021
@stephentoub stephentoub added this to the 6.0.0 milestone Jan 5, 2021
@GSPP
Copy link

GSPP commented Jan 7, 2021

Maybe void UnsafeStart(object? parameter = null)? What's the policy on using default parameter values in the BCL?

@bartonjs
Copy link
Member

bartonjs commented Jan 15, 2021

Video

Looks good as proposed, but we simplified it to use a default parameter.

namespace System.Threading
{
     public class Thread 
     {
          void UnsafeStart(object? parameter = null);
     }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jan 15, 2021
@stephentoub stephentoub self-assigned this Jan 15, 2021
@stephentoub
Copy link
Member

stephentoub commented Jan 15, 2021

Just realized there's a semantic difference between Start() and Start(object), with regards to error handling around whether the Thread was created with a ThreadStart or ParameterizedThreadStart delegate. So, we should stick with the original proposal of two overloads and maintain the same semantics for the Unsafe variants.

namespace System.Threading
{
     public class Thread 
     {
        public void UnsafeStart();
        public void UnsafeStart(object? parameter);
     }
}

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 15, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 16, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Feb 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Threading
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants