Skip to content

Commit

Permalink
PR feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@tmds
tmds committed Mar 4, 2021
1 parent fdbd7f0 commit f4e6480
Show file tree
Hide file tree
Showing 6 changed files with 56 additions and 61 deletions.
3 changes: 1 addition & 2 deletions src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
using Microsoft.Win32.SafeHandles;
using System.Collections.Generic;
using System.ComponentModel;
using System.Globalization;
using System.IO;
using System.IO.Pipes;
using System.Security;
Expand Down Expand Up @@ -760,7 +759,7 @@ internal static TimeSpan TicksToTimeSpan(double ticks)
private static Stream OpenStream(int fd, PipeDirection direction)
{
Debug.Assert(fd >= 0);
return new AnonymousPipeClientStream(direction, fd.ToString(CultureInfo.InvariantCulture));
return new AnonymousPipeClientStream(direction, new SafePipeHandle((IntPtr)fd, ownsHandle: true));
}

/// <summary>Parses a command-line argument string into a list of arguments.</summary>
Expand Down
1 change: 0 additions & 1 deletion src/libraries/System.Diagnostics.Process/tests/ProcessStreamReadTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,6 @@ async private Task<bool> WaitPipeSignal(PipeStream pipe, int millisecond)
}
}

[ActiveIssue("https://github.com/dotnet/runtime/issues/44329")]
[PlatformSpecific(~TestPlatforms.Windows)] // currently on Windows these operations async-over-sync on Windows
[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
public async Task ReadAsync_OutputStreams_Cancel_RespondsQuickly()
Expand Down
5 changes: 3 additions & 2 deletions src/libraries/System.IO.Pipes/src/Microsoft/Win32/SafeHandles/SafePipeHandle.Unix.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,7 @@ protected override void Dispose(bool disposing)
{
base.Dispose(disposing); // must be called before trying to Dispose the socket
_disposed = 1;
Socket? socket;
if (disposing && (socket = Volatile.Read(ref _pipeSocket)) != null)
if (disposing && Volatile.Read(ref _pipeSocket) is Socket socket)
{
socket.Dispose();
_pipeSocket = null;
Expand Down Expand Up @@ -84,6 +83,8 @@ private Socket CreatePipeSocket(bool ownsHandle = true)

socket = SetPipeSocketInterlocked(new Socket(new SafeSocketHandle(handle, ownsHandle)), ownsHandle);

// Double check if we haven't Disposed in the meanwhile, and ensure
// the Socket is disposed, in case Dispose() missed the _pipeSocket assignment.
if (_disposed == 1)
{
Volatile.Write(ref _pipeSocket, null);
Expand Down
88 changes: 43 additions & 45 deletions src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.Unix.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -244,60 +244,58 @@ private unsafe SocketError DoCloseHandle(bool abortive)
{
return SocketPal.GetSocketErrorForErrorCode(CloseHandle(handle));
}
else
{
// If abortive is not set, we're not running on the finalizer thread, so it's safe to block here.
// We can honor the linger options set on the socket. It also means closesocket() might return
// EWOULDBLOCK, in which case we need to do some recovery.
if (!abortive)
{
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"handle:{handle} Following 'non-abortive' branch.");

// Close, and if its errno is other than EWOULDBLOCK, there's nothing more to do - we either succeeded or failed.
errorCode = CloseHandle(handle);
if (errorCode != Interop.Error.EWOULDBLOCK)
{
return SocketPal.GetSocketErrorForErrorCode(errorCode);
}

// The socket must be non-blocking with a linger timeout set.
// We have to set the socket to blocking.
if (Interop.Sys.Fcntl.DangerousSetIsNonBlocking(handle, 0) == 0)
{
// The socket successfully made blocking; retry the close().
return SocketPal.GetSocketErrorForErrorCode(CloseHandle(handle));
}
// If abortive is not set, we're not running on the finalizer thread, so it's safe to block here.
// We can honor the linger options set on the socket. It also means closesocket() might return
// EWOULDBLOCK, in which case we need to do some recovery.
if (!abortive)
{
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"handle:{handle} Following 'non-abortive' branch.");

// The socket could not be made blocking; fall through to the regular abortive close.
// Close, and if its errno is other than EWOULDBLOCK, there's nothing more to do - we either succeeded or failed.
errorCode = CloseHandle(handle);
if (errorCode != Interop.Error.EWOULDBLOCK)
{
return SocketPal.GetSocketErrorForErrorCode(errorCode);
}

// By default or if the non-abortive path failed, set linger timeout to zero to get an abortive close (RST).
var linger = new Interop.Sys.LingerOption
// The socket must be non-blocking with a linger timeout set.
// We have to set the socket to blocking.
if (Interop.Sys.Fcntl.DangerousSetIsNonBlocking(handle, 0) == 0)
{
OnOff = 1,
Seconds = 0
};
// The socket successfully made blocking; retry the close().
return SocketPal.GetSocketErrorForErrorCode(CloseHandle(handle));
}

errorCode = Interop.Sys.SetLingerOption(handle, &linger);
#if DEBUG
_closeSocketLinger = SocketPal.GetSocketErrorForErrorCode(errorCode);
#endif
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"handle:{handle}, setsockopt():{errorCode}");
// The socket could not be made blocking; fall through to the regular abortive close.
}

switch (errorCode)
{
case Interop.Error.SUCCESS:
case Interop.Error.EINVAL:
case Interop.Error.ENOPROTOOPT:
case Interop.Error.ENOTSOCK:
errorCode = CloseHandle(handle);
break;

// For other errors, it's too dangerous to try closesocket() - it might block!
}
// By default or if the non-abortive path failed, set linger timeout to zero to get an abortive close (RST).
var linger = new Interop.Sys.LingerOption
{
OnOff = 1,
Seconds = 0
};

errorCode = Interop.Sys.SetLingerOption(handle, &linger);
#if DEBUG
_closeSocketLinger = SocketPal.GetSocketErrorForErrorCode(errorCode);
#endif
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"handle:{handle}, setsockopt():{errorCode}");

switch (errorCode)
{
case Interop.Error.SUCCESS:
case Interop.Error.EINVAL:
case Interop.Error.ENOPROTOOPT:
case Interop.Error.ENOTSOCK:
errorCode = CloseHandle(handle);
break;

return SocketPal.GetSocketErrorForErrorCode(errorCode);
// For other errors, it's too dangerous to try closesocket() - it might block!
}

return SocketPal.GetSocketErrorForErrorCode(errorCode);
}

private Interop.Error CloseHandle(IntPtr handle)
Expand Down
14 changes: 6 additions & 8 deletions src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncContext.Unix.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -787,15 +787,12 @@ public bool StartAsyncOperation(SocketAsyncContext context, TOperation operation
{
Trace(context, $"Enter");

if (!context.IsRegistered)
if (!context.IsRegistered && !context.TryRegister(out Interop.Error error))
{
if (!context.TryRegister(out Interop.Error error))
{
HandleFailedRegistration(context, operation, error);
HandleFailedRegistration(context, operation, error);

Trace(context, $"Leave, not registered");
return false;
}
Trace(context, "Leave, not registered");
return false;
}

while (true)
Expand Down Expand Up @@ -881,7 +878,8 @@ static void HandleFailedRegistration(SocketAsyncContext context, TOperation oper
// macOS: kevent returns EPIPE when adding pipe fd for which the other end is closed.
if (error == Interop.Error.EPIPE)
{
// Retry the operation.
// Because the other end close, we expect the operation to complete when we retry it.
// If it doesn't, we fall through and throw an Exception.
if (operation.TryComplete(context))
{
return;
Expand Down
6 changes: 3 additions & 3 deletions src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Unix.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ private static unsafe int SysWrite(SafeSocketHandle socket, ReadOnlySpan<byte> b

fixed (byte* b = &MemoryMarshal.GetReference(buffer))
{
sent = Interop.Sys.Write(socket, &b[offset], buffer.Length);
sent = Interop.Sys.Write(socket, b + offset, count);
if (sent == -1)
{
errno = Interop.Sys.GetLastError();
Expand All @@ -202,7 +202,7 @@ private static unsafe int SysSend(SafeSocketHandle socket, SocketFlags flags, Re
{
errno = Interop.Sys.Send(
socket,
&b[offset],
b + offset,
count,
flags,
&sent);
Expand All @@ -228,7 +228,7 @@ private static unsafe int SysSend(SafeSocketHandle socket, SocketFlags flags, Re
{
var iov = new Interop.Sys.IOVector
{
Base = &b[offset],
Base = b + offset,
Count = (UIntPtr)count
};

Expand Down

0 comments on commit f4e6480

Please sign in to comment.