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

[mono] Portable threadpool does not support async IO eg ThreadPoolBoundHandle on Windows #34582

Closed
MaximLipnin opened this issue Apr 6, 2020 · 13 comments · Fixed by #64834
Closed
Labels
area-VM-threading-mono help wanted [up-for-grabs] Good issue for external contributors os-windows runtime-mono specific to the Mono runtime
Milestone

Comments

@MaximLipnin
Copy link
Contributor

A lot of System.IO.Pipes.Tests fail with System.PlatformNotSupportedException : This API is specific to the way in which Windows handles asynchronous I/O, and is not supported on this platform. on windows.
The example of the output:

System.IO.Pipes.Tests.NamedPipeTest_Simple_ServerInOutRead_ClientInOutWrite.OperationsOnDisconnectedClient [FAIL]
System.PlatformNotSupportedException : This API is specific to the way in which Windows handles asynchronous I/O, and is not supported on this platform.
Stack Trace:
    _\src\libraries\System.Private.CoreLib\src\System\Threading\ThreadPoolBoundHandle.PlatformNotSupported.cs(25,0): at System.Threading.ThreadPoolBoundHandle.BindHandle(SafeHandle handle)
    _\src\libraries\System.IO.Pipes\src\System\IO\Pipes\PipeStream.Windows.cs(46,0): at System.IO.Pipes.PipeStream.InitializeAsyncHandle(SafePipeHandle handle)
    _\src\libraries\System.IO.Pipes\src\System\IO\Pipes\PipeStream.cs(101,0): at System.IO.Pipes.PipeStream.InitializeHandle(SafePipeHandle handle, Boolean isExposed, Boolean isAsync)
    _\src\libraries\System.IO.Pipes\src\System\IO\Pipes\NamedPipeServerStream.Windows.cs(122,0): at System.IO.Pipes.NamedPipeServerStream.Create(String pipeName, PipeDirection direction, Int32 maxNumberOfServerInstances, PipeTransmissionMode transmissionMode, PipeOptions options, Int32 inBufferSize, Int32 outBufferSize, PipeSecurity pipeSecurity, HandleInheritability inheritability, PipeAccessRights additionalAccessRights)
    _\src\libraries\System.IO.Pipes\src\System\IO\Pipes\NamedPipeServerStream.Windows.cs(49,0): at System.IO.Pipes.NamedPipeServerStream.Create(String pipeName, PipeDirection direction, Int32 maxNumberOfServerInstances, PipeTransmissionMode transmissionMode, PipeOptions options, Int32 inBufferSize, Int32 outBufferSize, HandleInheritability inheritability)
    _\src\libraries\System.IO.Pipes\src\System\IO\Pipes\NamedPipeServerStream.cs(85,0): at System.IO.Pipes.NamedPipeServerStream..ctor(String pipeName, PipeDirection direction, Int32 maxNumberOfServerInstances, PipeTransmissionMode transmissionMode, PipeOptions options, Int32 inBufferSize, Int32 outBufferSize, HandleInheritability inheritability)
    _\src\libraries\System.IO.Pipes\src\System\IO\Pipes\NamedPipeServerStream.cs(41,0): at System.IO.Pipes.NamedPipeServerStream..ctor(String pipeName, PipeDirection direction, Int32 maxNumberOfServerInstances, PipeTransmissionMode transmissionMode, PipeOptions options)
    _\src\libraries\System.IO.Pipes\tests\NamedPipeTests\NamedPipeTest.Simple.cs(761,0): at System.IO.Pipes.Tests.NamedPipeTest_Simple_ServerInOutRead_ClientInOutWrite.CreateNamedPipePair(PipeOptions serverOptions, PipeOptions clientOptions)
    _\src\libraries\System.IO.Pipes\tests\NamedPipeTests\NamedPipeTestBase.cs(42,0): at System.IO.Pipes.Tests.NamedPipeTestBase.CreateNamedPipePair()
    _\src\libraries\System.IO.Pipes\tests\NamedPipeTests\NamedPipeTest.Simple.cs(310,0): at System.IO.Pipes.Tests.NamedPipeTest_Simple.OperationsOnDisconnectedClient()

The whole namespace will be marked with ActiveIssue in #32592

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.IO untriaged New issue has not been triaged by the area owner labels Apr 6, 2020
@ghost
Copy link

ghost commented Apr 6, 2020

Tagging @carlossanlop as an area owner

@danmoseley
Copy link
Member

@MaximLipnin this is because Mono corelib is not built with <FeaturePortableThreadPool>true</FeaturePortableThreadPool>. Should it be?
cc @kouvel

@MaximLipnin
Copy link
Contributor Author

@danmosemsft Thanks for clarification, but please correct me if I'm wrong - a quick search shows that this flag is activated in Mono's SPC

@kouvel
Copy link
Member

kouvel commented Apr 7, 2020

The portable thread pool implementation currently doesn't support BindHandle. I guess that Mono would inherit the PNSE implementation here:

<ItemGroup Condition="'$(FeaturePortableThreadPool)' == 'true'">
<Compile Include="$(MSBuildThisFileDirectory)System\AppContextConfigHelper.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Threading\ThreadPool.Portable.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Threading\ThreadPoolBoundHandle.PlatformNotSupported.cs" />

I guess IO completion stuff may need to be added to the portable implementation on Windows to make BindHandle work there, I don't know if Mono has an alternative implementation it can fork to easily at the moment for those. Probably would be best to add that to the portable implementation.

@marek-safar
Copy link
Contributor

marek-safar commented Apr 8, 2020

<FeaturePortableThreadPool>true</FeaturePortableThreadPool>

This is correct.

Could the tests be guarded with IsBindHandleSupported instead of Windows only or is the BindHandle support something your team can fix?

I know @jkotas would like to use portable TP for CoreCLR too and this could be a blocker.

@jkotas
Copy link
Member

jkotas commented Apr 8, 2020

@jkotas would like to use portable TP for CoreCLR too and this could be a blocker.

Adding implementation for IO completion stuff (ie BindHandle and friends) is one of the things that will need to be fixed to fully switch to portable TP for CoreCLR on Windows.

To understand the priority: Is Mono going to ship as officially supported on Windows for .NET 5?

@marek-safar
Copy link
Contributor

It's not high on our priority list to support IO completion stuff anywhere (including Windows)

@danmoseley
Copy link
Member

To understand the priority: Is Mono going to ship as officially supported on Windows for .NET 5?

I am interested in this too as it helps us understand what to do about these mono-on-Windows bugs. My assumption is that there is value in getting this test pass clean, regardless.

cc @steveisok

@danmoseley danmoseley added runtime-mono specific to the Mono runtime os-windows labels Apr 10, 2020
@danmoseley danmoseley changed the title [mono] Tests failed due to PNSE on windows: System.IO.Pipes.Tests [mono] Portable threadpool does not support async IO eg ThreadPoolBoundHandle Apr 10, 2020
@carlossanlop carlossanlop added help wanted [up-for-grabs] Good issue for external contributors and removed untriaged New issue has not been triaged by the area owner labels Apr 10, 2020
@ericstj
Copy link
Member

ericstj commented Apr 13, 2020

Libraries rely on this API in order to create an IO completion ports. This is necessary for any of our async IO implementations to work on Windows. I agree with @jkotas here: if you care about running mono on Windows I wouldn't leave this as throwing.

@jkotas
Copy link
Member

jkotas commented Apr 13, 2020

One option to consider is to use Windows OS built-in threadpool that comes with IO completion port support. It is what CoreRT does: https://github.com/dotnet/corert/blob/master/src/System.Private.CoreLib/src/System/Threading/ThreadPool.Windows.cs . It comes with trade-offs, e.g. Windows OS built-in threadpool has different heuristics.

@SamMonoRT SamMonoRT changed the title [mono] Portable threadpool does not support async IO eg ThreadPoolBoundHandle [mono] Portable threadpool does not support async IO eg ThreadPoolBoundHandle on Windows Jul 26, 2021
@SamMonoRT
Copy link
Member

Moving to 7.0.0

@lateralusX
Copy link
Member

I guess IO completion stuff may need to be added to the portable implementation on Windows to make BindHandle work there, I don't know if Mono has an alternative implementation it can fork to easily at the moment for those. Probably would be best to add that to the portable implementation.

@kouvel, any plans to actually implement support in portable thread pool so we can get this working on Mono?

@kouvel
Copy link
Member

kouvel commented Jan 20, 2022

any plans to actually implement support in portable thread pool so we can get this working on Mono?

Yes I'm planning on adding support for BindHandle in the portable thread pool soon.

@ghost ghost added in-pr There is an active PR which will close this issue when it is merged and removed in-pr There is an active PR which will close this issue when it is merged labels Feb 7, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 8, 2022
kouvel added a commit to kouvel/runtime that referenced this issue Feb 13, 2022
kouvel added a commit to kouvel/runtime that referenced this issue Mar 3, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Mar 4, 2022
akoeplinger added a commit to akoeplinger/runtime that referenced this issue Mar 11, 2022
akoeplinger added a commit that referenced this issue Mar 12, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Apr 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-VM-threading-mono help wanted [up-for-grabs] Good issue for external contributors os-windows runtime-mono specific to the Mono runtime
Projects
None yet
Development

Successfully merging a pull request may close this issue.