basic support for TCP fast open #99490

wfurt · 2024-03-10T03:38:19Z

There are three somewhat separated parts here:

This adds API approved in TCP Fast Open implementation? #1476. It is just simple enum so we can do all platforms right.
bring platform parity for patten described in Socket connect with payload via SocketAsyncEventArgs fails #79654 (and TCP Fast Open implementation? #1476 as well)

using var client = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
var saea = new SocketAsyncEventArgs();
saea.RemoteEndPoint = new IPEndPoint(IPAddress.Loopback, 42);
saea.SetBuffer(new byte[42]); // <== the presence of the buffer is what triggers the failure

Tis currently works on Windows using connectex. With this PR it will light up on macOS using connectx and on Linux using combination of Connect & Send. Test added in #79669 is now portable across platforms.

With the work above, we will attempt TFO if combined. On Windows connectex can already do that so as connectx on macOS. To make it work consistently across platforms I used TCP_FASTOPEN_CONNECT. That is available since Kernel 4.11 and that should cover all supported releases AFAIK. This is still best effort as the actual behavior will change based on /proc setting as well as TCP cookie availability. If either one is missing we will simply fallback to behavior described in 2)

dotnet-issue-labeler · 2024-03-10T03:38:25Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Unix.cs

antonfirsov

Have you validated this with packet captures on all 3 platforms?

I don't know how it works on Mac, but note that this does not add support for server-side TFO on Linux therefore it's not a full implementation of #1476. We should either document this limitation or implement (& validate) server-side TFO.

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Unix.cs

src/native/libs/System.Native/pal_networking.c

antonfirsov · 2024-03-12T13:37:28Z

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketOptionName.cs

@@ -137,6 +137,7 @@ public enum SocketOptionName
        BsdUrgent = 2,
        Expedited = 2,
        TcpKeepAliveRetryCount = 16,
+        FastOpen = 15,


We should document the concerns (=fully controlled datapath needed) and the limitations around this flag.

yes, I have no problem with good description. Even the man page for Linux talks about differences based on cookie availability.

wfurt · 2024-03-12T21:08:28Z

I don't know how it works on Mac, but note that this does not add support for server-side TFO on Linux

what do you think is missing?

wfurt · 2024-03-12T21:17:07Z

Have you validated this with packet captures on all 3 platforms?

I did macOS & Linux since Windows was reported working.

antonfirsov · 2024-03-12T21:31:06Z

what do you think is missing?

With server side sockets (listener/accept) TCP_FASTOPEN_CONNECT will not make it set a TFO cookie (and it's only being set on connect). The server side needs TCP_FASTOPEN enabled to operate. (This is Linux, didn't research how it works on Mac.)

wfurt · 2024-03-12T21:46:19Z

what do you think is missing?

With server side sockets (listener/accept) TCP_FASTOPEN_CONNECT will not make it set a TFO cookie (and it's only being set on connect). The server side needs TCP_FASTOPEN enabled to operate. (This is Linux, didn't research how it works on Mac.)

yes, This is what the part 1) does e.g. the new SocketOption

case SocketOptionName_SO_TCP_FASTOPEN:
           *optName = TCP_FASTOPEN;
            return true;

antonfirsov · 2024-03-12T22:11:42Z

case SocketOptionName_SO_TCP_FASTOPEN:

That logic is only triggered for mapping SocketOptionName.FastOpen to TCP_FASTOPEN in SystemNative_GetSockOpt. Maybe I'm missing something, but I don't see a logic enabling TCP_FASTOPEN for listener sockets after listen.SetSocketOption(SocketOptionLevel.Tcp, SocketOptionName.FastOpen, 1) calls.

wfurt · 2024-03-12T22:23:31Z

That logic is only triggered for mapping SocketOptionName.FastOpen to TCP_FASTOPEN in SystemNative_GetSockOpt. Maybe I'm missing something, but I don't see a logic enabling TCP_FASTOPEN for listener sockets after listen.SetSocketOption(SocketOptionLevel.Tcp, SocketOptionName.FastOpen, 1) calls.

This part I don't understand. The rest works just like any other option. We map the .NET value to platform value and call setsockopt. No additional code is needed besides the mapping.

antonfirsov · 2024-03-12T22:46:59Z

No additional code is needed besides the mapping.

Ok, it should be good (I missed part of the overall option setting logic). We still need the option roundtrip tests though.

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Unix.cs

antonfirsov · 2024-05-03T13:44:56Z

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketOptionName.cs

        Expedited = 2,
-        TcpKeepAliveRetryCount = 16,
+        /// <summary>
+        /// This enables TCP Fast Open as defined in RFC-7413. The actual observed behavior depend on OS configuration and state of kernel TCP cookie cache.


I strongly believe that we need to document the risks of TFO in an easily discoverable way, including the fact that it's only recommended for fully controlled datapaths. I think this summary - or if it's valid for enum members - a remarks section would be a good place.

What dog you suggest? Remarks probably should be edited directly in doc repo...?
I was thinking about it more and perps we can add something like

This enables TCP Fast Open as defined in RFC-7413. The actual observed behavior depend on OS configuration and state of kernel TCP cookie cache. Enabling TFO can impact interoperability and casue connectivity issues.

This enables TCP Fast Open as defined in RFC-7413. The actual observed behavior depend on OS configuration and state of kernel TCP cookie cache. Enabling TFO can impact interoperability and casue connectivity issues.

I would be ok with changing the summary to the text above & opening an issue against https://github.com/dotnet/dotnet-api-docs/ to track adding more detailed information to an appropriate place, SocketOptionName enum remarks maybe.

wfurt · 2024-05-14T15:58:44Z

dotnet/dotnet-api-docs#9910 created to track adding remarks.

antonfirsov

LGTM assuming the test failure will be addressed.

antonfirsov · 2024-05-22T19:40:55Z

src/libraries/System.Net.Sockets/tests/FunctionalTests/SocketAsyncEventArgsTest.cs

@@ -943,4 +1088,41 @@ public async Task SendTo_DifferentEP_Success(bool ipv4)
            Assert.Equal(sendBuffer.Length, result.ReceivedBytes);
        }
    }
+
+    internal static class ConnectExtensions


Nit: it might be worth to move this into a separate file.

antonfirsov · 2024-05-22T19:41:58Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2024-05-22T19:42:12Z

Azure Pipelines successfully started running 1 pipeline(s).

wfurt · 2024-05-23T14:58:20Z

all the outer loop tests failures seems unrelated.

antonfirsov · 2024-05-24T17:35:09Z

@wfurt the failure I mentioned is not in outerloop. It's still there in the latest runtime run, and there were no commits addressing it: https://github.com/dotnet/runtime/pull/99490/checks?check_run_id=25312010880

wfurt · 2024-05-24T17:37:30Z

it was clean on the last run - but I guess that was lock. it should be fixed by #102668 e.g. #102650

* initial drop * update * cleanup * err * 'test' * windows * sync * feedback & updates * macos * windows * feedback * await * docs * note * macos --------- Co-authored-by: Ubuntu <toweinfu@toweinfu-ubu22.c5goow0wwwee5hembzptmbtr0h.xx.internal.cloudapp.net>

wfurt added 4 commits March 8, 2024 15:12

initial drop

d63bd81

Merge branch 'main' of https://github.com/dotnet/runtime into tfo

e0ab1d1

update

cbb8a2b

cleanup

e52bd29

dotnet-issue-labeler bot added area-System.Net.Sockets new-api-needs-documentation labels Mar 10, 2024

wfurt added 2 commits March 9, 2024 20:25

err

d8c735a

'test'

b1dabaf

gfoidl reviewed Mar 11, 2024

View reviewed changes

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Unix.cs Outdated Show resolved Hide resolved

windows

945e565

build-analysis bot mentioned this pull request Mar 11, 2024

Segmentation fault in System.Text.RegularExpressions.Tests #93206

Closed

sync

eee978c

This was referenced Mar 12, 2024

Tracking issue for CI build timeouts #76454

Closed

AzDo uploading failure: "Blob is incomplete" #92148

Open

File upload failed even after retry dotnet/dnceng#1916

Open

wfurt self-assigned this Mar 12, 2024

wfurt requested review from stephentoub and a team March 12, 2024 04:33

wfurt added this to the 9.0.0 milestone Mar 12, 2024

wfurt marked this pull request as ready for review March 12, 2024 04:34

antonfirsov reviewed Mar 12, 2024

View reviewed changes

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Unix.cs Outdated Show resolved Hide resolved

wfurt added 2 commits May 2, 2024 19:53

Merge branch 'main' of https://github.com/dotnet/runtime into tfo

850bdbb

docs

a80fbff

antonfirsov reviewed May 3, 2024

View reviewed changes

wfurt added 2 commits May 6, 2024 22:27

note

390db80

Merge branch 'main' of https://github.com/dotnet/runtime into tfo

f2dce6e

build-analysis bot mentioned this pull request May 7, 2024

System.Data.OleDb.Tests timeout in net48 x86 Release leg #87783

Open

wfurt added 2 commits May 14, 2024 08:57

macos

d6a0fa9

Merge branch 'main' of https://github.com/dotnet/runtime into tfo

615de25

build-analysis bot mentioned this pull request May 15, 2024

NativeAOT legs timing out in CI #102239

Closed

antonfirsov approved these changes May 22, 2024

View reviewed changes

wfurt merged commit e377f16 into dotnet:main May 23, 2024
159 of 167 checks passed

wfurt deleted the tfo branch May 23, 2024 14:58

rzikm mentioned this pull request May 24, 2024

[Test Failure] System.Net.Sockets.Tests.SocketAsyncEventArgsTest.Connect_WithData_OK #102650

Closed

wfurt mentioned this pull request May 28, 2024

don't return ConnectOperation as we are not pooling it #102776

Merged

github-actions bot locked and limited conversation to collaborators Jun 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

basic support for TCP fast open #99490

basic support for TCP fast open #99490

wfurt commented Mar 10, 2024 •

edited

Loading

dotnet-issue-labeler bot commented Mar 10, 2024

antonfirsov left a comment •

edited

Loading

antonfirsov Mar 12, 2024

wfurt Mar 12, 2024

wfurt commented Mar 12, 2024

wfurt commented Mar 12, 2024

antonfirsov commented Mar 12, 2024

wfurt commented Mar 12, 2024

antonfirsov commented Mar 12, 2024

wfurt commented Mar 12, 2024

antonfirsov commented Mar 12, 2024 •

edited

Loading

antonfirsov May 3, 2024 •

edited

Loading

wfurt May 6, 2024

antonfirsov May 6, 2024

wfurt commented May 14, 2024

antonfirsov left a comment

antonfirsov May 22, 2024

antonfirsov commented May 22, 2024

azure-pipelines bot commented May 22, 2024

wfurt commented May 23, 2024

antonfirsov commented May 24, 2024

wfurt commented May 24, 2024

basic support for TCP fast open #99490

basic support for TCP fast open #99490

Conversation

wfurt commented Mar 10, 2024 • edited Loading

dotnet-issue-labeler bot commented Mar 10, 2024

antonfirsov left a comment • edited Loading

Choose a reason for hiding this comment

antonfirsov Mar 12, 2024

Choose a reason for hiding this comment

wfurt Mar 12, 2024

Choose a reason for hiding this comment

wfurt commented Mar 12, 2024

wfurt commented Mar 12, 2024

antonfirsov commented Mar 12, 2024

wfurt commented Mar 12, 2024

antonfirsov commented Mar 12, 2024

wfurt commented Mar 12, 2024

antonfirsov commented Mar 12, 2024 • edited Loading

antonfirsov May 3, 2024 • edited Loading

Choose a reason for hiding this comment

wfurt May 6, 2024

Choose a reason for hiding this comment

antonfirsov May 6, 2024

Choose a reason for hiding this comment

wfurt commented May 14, 2024

antonfirsov left a comment

Choose a reason for hiding this comment

antonfirsov May 22, 2024

Choose a reason for hiding this comment

antonfirsov commented May 22, 2024

azure-pipelines bot commented May 22, 2024

wfurt commented May 23, 2024

antonfirsov commented May 24, 2024

wfurt commented May 24, 2024

wfurt commented Mar 10, 2024 •

edited

Loading

antonfirsov left a comment •

edited

Loading

antonfirsov commented Mar 12, 2024 •

edited

Loading

antonfirsov May 3, 2024 •

edited

Loading