Start thread pool worker threads in the default execution context

[kouvel]

The execution context transfers to new threads, and this creates an extra stack frame on the thread and may cause AsyncLocal value change notifications to be sent unnecessarily. Switched to the default context before creating a worker thread.

[danmoseley]

Is it possible to test this (even if it's not a certain result)

[kouvel]

The Dispatch method that kicks off work items force-clears the EC before work items are run, so it wouldn't be user-visible that way. It may be possible through AsyncLocal value-change notifications, but technically it's not incorrect to send those notifications (i.e. not necessarily a bug, but it may be a behavior difference from the native thread pool). I'll try to test with the notifications.

[kouvel]

Actually it may be a bug that this is fixing. AsyncLocal value change notifications can have side-effects like impersonation, the force-clearing that Dispatch does in the beginning does would not undo that since it doesn't send value-change notifications (the AsyncLocal value would be correct, but the side-effect would not have been undone.

[jkotas]

Do we have tests that validate that new threads inherit execution context of the parent thread? I do not see the code that captures the execution context of parent thread in Mono, and no bugs on it either.

[janvorli]

LGTM, thank you!

[kouvel]

Do we have tests that validate that new threads inherit execution context of the parent thread?

Doesn't look like it. I added a test now, but will need to check the implementation in Mono.

[kouvel]

The EC flowing into thread pool threads did turn out to be a bug with visible unintended consequences, added a test for it. Also fixed the same thing for gate thread and wait thread creation.

[davidfowl]

Can't we add a no-capture flag to the thread constructor to avoid capturing the execution context?

[kouvel]

Yea that's an option, it's the Start methods that flow the EC, maybe we could mimic the UnsafeQueue... methods on ThreadPool and call them Thread.UnsafeStart, which would not flow the EC?

CC @stephentoub

[kouvel]

Does seem a bit odd though

[kouvel]

I suppose Thread.UnsafeStart doesn't have to be a public API for the time being, it could just be used as the internal implementation for now, if that's preferred, until it is requested to be a public API. Thoughts?

[jkotas]

Yes, make it internal API for this PR

[davidfowl]

Yep sounds good

[kouvel]

FYI for new reviewers there will be more changes coming I'll send an update once it's ready for review

[kouvel]

Updated to add internal Thread.UnsafeStart and used that instead, the code is simpler and cleaner so it's nice. Changes are ready for review, please take a look.

[davidfowl]

Looks great. I assume we should do the same for timer threads in the portable timer case but those threads aren't created lazily AFAIK

[kouvel]

I have updated the portable timer now as well. The timer thread is created lazily the first time a timer is scheduled.