Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

Do not inherit socket handles #2944

Merged
merged 1 commit into from
Oct 19, 2018
Merged

Do not inherit socket handles #2944

merged 1 commit into from
Oct 19, 2018

Conversation

Tratcher
Copy link
Member

#2789 If the accept socket's handle gets inherited by child processes, then it's not fully closed until all of the child processes are closed.

Connected sockets don't seem to be affected.

@Tratcher Tratcher added this to the 2.2.0-preview3 milestone Sep 18, 2018
@Tratcher Tratcher self-assigned this Sep 18, 2018
@Tratcher Tratcher requested a review from halter73 September 18, 2018 23:28

var processInfo = new ProcessStartInfo
{
FileName = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "cmd.exe" : "bash",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might as well using sh instead of bash since the former is required by POSIX even if it happens to be bash.

@Tratcher
Copy link
Member Author

@dotnet-bot test Windows Debug x64 Build
@dotnet-bot test Windows Release x64 Build

@Tratcher
Copy link
Member Author

@dotnet-bot test OSX 10.12 Debug x64 Build
@dotnet-bot test OSX 10.12 Release x64 Build

@Tratcher
Copy link
Member Author

Hm, VI works on linux but not mac. I may call that good enough for this test and skip Mac.

@Tratcher
Copy link
Member Author

@dotnet-bot test Windows Debug x64 Build


namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests
{
[OSSkipCondition(OperatingSystems.MacOSX)] // Not applicable, and vi doesn't work there.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure handle inheritance isn't an issue on macOS?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the corefx issue was clear that this is only an issue for Windows.

namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests
{
[OSSkipCondition(OperatingSystems.MacOSX)] // Not applicable, and vi doesn't work there.
public class HandleInheritanceTest : TestApplicationErrorLoggerLoggedTest
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: HandleInheritanceTests

@Tratcher
Copy link
Member Author

Tratcher commented Sep 28, 2018

Hmm, the Listen test consistently fails on Mac Sockets, and the Connection test is flaky on Windows Libuv.
@dotnet-bot test OSX 10.12 Debug Build
@dotnet-bot test OSX 10.12 Release Build
@dotnet-bot test Windows Debug x64 Build
@dotnet-bot test Windows Release x64 Build

@Tratcher
Copy link
Member Author

Tratcher commented Oct 2, 2018

Ok, I tracked down the Mac issue.

The linux equivilent of what we did on windows is https://legacy.python.org/dev/peps/pep-0433/#socket-socket
But these APIs doen't exist on MacOS, or at least didn't exist a few years ago when other frameworks were dealing with this.
rust-lang/rust#24237
https://github.com/libuv/libuv/blob/81c77427afb39b6fa6c0ecffdc1c1e2b3dde7aea/src/unix/core.c#L417-L424
Regardless, those workarounds primarily pass a flag to the socket constructor which we cannot do.

I'm going to go back to skipping this test on Mac since we don't have a way to fix it there. Still need to figure out what's flaky with Windows Libuv.

@tmds
Copy link
Contributor

tmds commented Oct 4, 2018

On mac and Linux corefx does this by default: https://github.com/dotnet/corefx/blob/master/src/Native/Unix/System.Native/pal_networking.c#L1292-L1307

Perhaps corefx should do this for Windows sockets too?
The best implementation is when you can pass the flag when you create the socket. Otherwise there is a race-condition when someone creates a child between socket construction and setting the flag.

@Tratcher
Copy link
Member Author

Tratcher commented Oct 4, 2018

@tmds what's the expected behavior of FD_CLOEXEC? From the name it sounds like close-on-process-exit. It isn't working in my Mac tests (currently disabled). Note my tests don't close the parent process, only the socket in the parent process.

@tmds
Copy link
Contributor

tmds commented Oct 4, 2018

Cloexec causes the handle not to be inherited by child processes. Exec refers to the system call to execute a program. When the child process execs, that handle is closed for the child.

@Tratcher
Copy link
Member Author

Tratcher commented Oct 4, 2018

When the child process execs, that handle is closed for the child.

Ah, but the duplicated handle remains open in the parent process even if I close the original Socket? That would explain why it's not working on Mac, but doesn't explain why Linux is working.

@tmds
Copy link
Contributor

tmds commented Oct 4, 2018

The listen socket is not duplicated to the children, so when the parent closes it, the server is no longer accepting connections. That explains why it works on Linux. I don't know why it doesn't work on Mac. Mac has limited cloexec apis compared to Linux. Maybe some things are not really supported.

For Windows, I think it makes sense that corefx sockets also by default would not be inherited by children.

@muratg muratg modified the milestones: 2.2.0-preview3, 2.2.0 Oct 10, 2018
@Tratcher
Copy link
Member Author

Updated. I've removed the flaky connected socket test, we're not modifying that scenario and the flakiness seems to be unrelated. The listen test is disabled for Mac.

Note this is considered to be a temporary mitigation. I'll follow up with corefx for a more comprehensive xplat fix in 3.0.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants