dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.37k stars 4.75k forks source link

Increase file descriptor limit in NativeAOT app process #82719

Open akoeplinger opened 1 year ago

akoeplinger commented 1 year ago

CoreCLR and soon Mono (after https://github.com/dotnet/runtime/pull/82429) call a function that will increase the maximum open file descriptors via a function like this, should NativeAOT do the same? https://github.com/dotnet/runtime/blob/2bdc3cb8dd3500aa2b3a51b558782560b26b5973/src/coreclr/pal/src/init/pal.cpp#L1087-L1093

/cc @dotnet/ilc-contrib

ghost commented 1 year ago

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas See info in area-owners.md if you want to be subscribed.

Issue Details
Both CoreCLR and Mono call a function that will increase the maximum open file descriptors via a function like this, should NativeAOT do the same? https://github.com/dotnet/runtime/blob/2bdc3cb8dd3500aa2b3a51b558782560b26b5973/src/coreclr/pal/src/init/pal.cpp#L1087-L1093 /cc @dotnet/ilc-contrib
Author: akoeplinger
Assignees: -
Labels: `area-NativeAOT-coreclr`
Milestone: -
VSadov commented 1 year ago

It probably makes sense, just for parity purposes. I am curious - are there specific reasons for this in CoreCLR/Mono or is it "just in case"? Any interesting history for why the call was introduced?

jkotas commented 1 year ago

FWIW, this is disabled on Alpine and nobody ever complained about it there: https://github.com/dotnet/runtime/blob/e9b9489f41cface8f537f87ab1e187c9244c5ea9/src/coreclr/pal/src/CMakeLists.txt#L95-L96

akoeplinger commented 1 year ago

I don't know about the historical context (the function was already part of the initial CoreCLR github commit) but we recently saw issues if this isn't done during NuGet restore: https://github.com/dotnet/runtime/issues/82428

jkotas commented 1 year ago

historical context

This was part of the original Rotor PAL: https://github.com/SSCLI/sscli_20021101/blob/77d46e0f04f52052a12ac40ce2cf96712c934b3c/pal/unix/init/pal.c#L629 .

VSadov commented 1 year ago

I remember there was a bug fix for when we could temporarily have too many fd's. I'll try finding details.

VSadov commented 1 year ago

Maybe it was to compensate for https://github.com/dotnet/runtime/issues/47859 ? (Assembly.Load opens /dev/zero in linux and does not close it)

It was fixed in https://github.com/dotnet/runtime/pull/48140

VSadov commented 1 year ago

At the time of Rotor using /dev/zero for anonymous memory mapping was likely the "right" way.

akoeplinger commented 1 year ago

Given we're seeing https://github.com/dotnet/runtime/issues/82428 when the limit is not increased I think it still makes sense to have it, though you could argue that with NativeAOT in library mode maybe the embedding application should have the final say.

VSadov commented 1 year ago

The scenario with NuGet looks real. The app has a scenario that may open 1K+ files and gets away with that on CorCLR, making this a compat issue.

I think we should increase the limit on NativeAOT and perhaps on Alpine too, unless the reason (debugging is broken) is still there.

VSadov commented 1 year ago

NativeAOT in library mode maybe the embedding application should have the final say.

No compat concerns in this case. It might be strange if library does this, especially if dynamically loaded.

agocke commented 1 year ago

It seems reasonable to match the CoreCLR behavior here.